You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/03/23 15:53:20 UTC
[GitHub] [drill] eevanwong opened a new pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
eevanwong opened a new pull request #2191:
URL: https://github.com/apache/drill/pull/2191
# [DRILL-7882](https://issues.apache.org/jira/projects/DRILL/issues/DRILL-7882?filter=updatedrecently): Fix LGTM Alerts in common folder
#[DRILL-7883](https://issues.apache.org/jira/projects/DRILL/issues/DRILL-7883?filter=updatedrecently): Fix LGTM Alerts in contrib folder
## Description
This is fairly long,
I did not want to have the actual alerts here as well due to the fact that some of them would refer to previous lines of code in the file. This is why I have the links instead.
Those marked ??? are alerts I did not understand but suppressed anyways
common section
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/config/DrillProperties.java?sort=name&dir=ASC&mode=heatmap
DrillProperties.java
line 115
???
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/HistoricalLog.java?sort=name&dir=ASC&mode=heatmap
HistoricalLog.java
Line 122 & 129
Suppressed b/c they were comments
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/common/src/main/java/org/apache/drill/common/exceptions/UserException.java?sort=name&dir=ASC&mode=heatmap
UserException.java
Line 481
Suppressed first b/c suggested solution was not valid, format fn needs those 2 parameters
Line 643
Based on the comments I believe this is not utilized in the main functionality of drill.
alert suppressed
contrib section
in format-excel
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java#x4f0d3a45123fb50b:1
excelbatchreader.java
line 280
suppressed because if value is null it is already handled in switch statement
in format-hdf5
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/HDF5BatchReader.java?sort=name&dir=ASC&mode=heatmap
HDF5BatchReader.java
line 593
Ignored warning as the format call just ignores additional arguments
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5ByteDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5ByteDataWriter.java
line 71
Warning Ignored as the ++ does not do anything b/c it only applies the change after return
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5DoubleDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5DoubleDataWriter.java
line 69
Same reason as the above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5FloatDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5FloatDataWriter.java
line 69
Same reason as the above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5IntDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5IntDataWriter.java
line 70
Same reason as the above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5LongDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5LongDataWriter.java
line 69
Same reason as the above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5SmallIntDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5SmallIntDataWriter.java
line 71
Same reason as the above
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-hdf5/src/main/java/org/apache/drill/exec/store/hdf5/writers/HDF5TimestampDataWriter.java?sort=name&dir=ASC&mode=heatmap
HDF5TimeSTampDataWriter
line 48
Same reason as the above
in format-img
https://lgtm.com/projects/g/apache/drill/snapshot/5ba93040059efc36da020f3cfd1ad31489e2e55e/files/contrib/format-image/src/main/java/org/apache/drill/exec/store/image/GenericMetadataDescriptor.java?sort=name&dir=ASC&mode=heatmap
GenericMetadataDescriptor.java
line 82,83,84
Suppress warnings as theyre required to display in format(needs objects).
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-image/src/main/java/org/apache/drill/exec/store/image/ImageDirectoryProcessor.java?sort=name&dir=ASC&mode=heatmap
ImageDirectoryProcessor.java
line 124
Error was suppressed (initialized with null value does not matter)
in format-maprdb
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/planner/index/MapRDBStatistics.java?sort=name&dir=ASC&mode=heatmap
line 777
Added if (pattern != null) statement to avoid potential NPE error
line 801
same as above
line 807
if statement but for escape
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/binary/BinaryTableGroupScan.java?sort=name&dir=ASC&mode=heatmap
BinaryTableGroupScan.java
line 190
Suppressed, Does not seem to be particularly needed for functionality of drill(?)
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonTableGroupScan.java?sort=name&dir=ASC&mode=heatmap
JsonTableGroupScan
JsonTableGroupScan.java
Line 380
suppressed
line 493
suppressed because its comments
line 520
format call gets 5 but only supplies 4, but the call just ignores the additional parameters
suppressed
line 527, 528, 541, 542, 632
All are comments, suppressed
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/JsonTableRangePartitionFunction.java?sort=name&dir=ASC&mode=heatmap
JsonTableRangePartitionFunction.java
Line 46
???
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/json/MaprDBJsonRecordReader.java?sort=name&dir=ASC&mode=heatmap
MaprDBJsonRecordReader.java
Line 431
???
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
MapRDBFormatPluginConfig.java
Line 28
???
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBGroupScan.java?sort=name&dir=ASC&mode=heatmap
MapRDBGroupScan.java
line 255
format call just ignores additional args anyways
suppressed
line 324
if discover is null it already logs an error
suppressed
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBTableCache.java?sort=name&dir=ASC&mode=heatmap
MapRDBTableCache.java
Line 73
Suppressed b/c there is a null guard beforehand
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/streams/StreamsFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
StreamsFormatpluginConfig.javas
line 27
???
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/TableFormatPluginConfig.java?sort=name&dir=ASC&mode=heatmap
TableFormatPluginConfig.java
Line 22
???
In format-xml
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/format-xml/src/main/java/org/apache/drill/exec/store/xml/XMLBatchReader.java?sort=name&dir=ASC&mode=heatmap
XMLBatchReader.java
line 94
suppressed, additional arg for format call are just ignored
In storage-druid
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-druid/src/main/java/org/apache/drill/exec/store/druid/DruidGroupScan.java?sort=name&dir=ASC&mode=heatmap
DruidGroupScan.java
line 201
Added L for long type specification
In storage-hbase
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseUtils.java?sort=name&dir=ASC&mode=heatmap
HBaseUtils.java
line 79
Suppressed, Dont really see the harm of it due to it just throwing a print statement
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java?sort=name&dir=ASC&mode=heatmap
HBasePersistentStore.java
line 201
suppressed b/c its in try catch statement
In storage-kafka
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaPartitionScanSpec.java?sort=name&dir=ASC&mode=heatmap
KafkaPartitionScanSpec.java
line 25
???
In storage-kudu
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-kudu/src/main/java/org/apache/drill/exec/store/kudu/KuduGroupScan.java?sort=name&dir=ASC&mode=heatmap
KuduGroupScan.java
line 210
Added L for long type specification
In storage-splunk
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/storage-splunk/src/main/java/org/apache/drill/exec/store/splunk/SplunkBatchReader.java?sort=name&dir=ASC&mode=heatmap
SplunkBatchReader.java
line 232
???
In udfs
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/udfs/src/main/java/org/apache/drill/exec/udfs/CryptoFunctions.java?sort=name&dir=ASC&mode=heatmap
CryptoFunctions.java
line 288
???
line 339
???
https://lgtm.com/projects/g/apache/drill/snapshot/e2a0925dd18aacf3a5657acd738f89a63a3b8576/files/contrib/udfs/src/main/java/org/apache/drill/exec/udfs/NetworkFunctions.java?sort=name&dir=ASC&mode=heatmap
NetworkFunctions.java
Line 434
???
## Documentation
N/A
## Testing
Due to how LGTM alerts work, if it isnt on the actual project, we can't see if it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-811867003
@eevanwong Thanks. I don't like the false-positive (incorrect). It can take a lot of time.
1. About overriding `hashCode` and `equals`. I holding the review. If possible to count the total number of alerts related to this issues?
2. Nullable. It's related to the heap memory of JVM (Garbage Collection Algorithm)? Ensure the variable is not null for ever?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-814914037
Hey so, a bit of an update, for hash code and equals, tuns out you in fact do need both.
> Yes, this is a legitimate concern. The contract between hashCode and equals is that equal instances must have the same hashCode. The reverse is not necessary, but high quality hashCode algorithms should avoid this as much as possible.
According to javadoc for the equals method:
Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.
https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object)
I suppose this might be a larger issue? I'm not very adept at Java so I myself am not very aware of proper conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-812868523
@eevanwong Thanks for doing this. This job is not easy, because there are many things that can be done or not. Is possible to contact the LGTM.com request a reply about these issues and our thoughts (with the odd recommendations)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-846280627
Yeah...
If you can roll back what you just did, here's how I rebase.
git checkout master
git fetch upstream
git rebase upstream/master
git checkout <your branch>
git rebase master
There may be an easier way, but that way get's the job done.
-- C
> On May 21, 2021, at 5:46 PM, Evan W. ***@***.***> wrote:
>
>
> Sorry, been awhile, I got hard sidetracked by school and whatnot. That being said I think I screwed up the rebase. :/
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub <https://github.com/apache/drill/pull/2191#issuecomment-846278755>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKB7PSNRR2XICHYMCIUSLLTO3ICHANCNFSM4ZVOWFHQ>.
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808392405
The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with (via null guard or logger); however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805086618
Errors in CI involve drillbuf.java, DrillFileSystem.java.
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on a change in pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on a change in pull request #2191:
URL: https://github.com/apache/drill/pull/2191#discussion_r612096730
##########
File path: contrib/format-xml/src/main/java/org/apache/drill/exec/store/xml/XMLBatchReader.java
##########
@@ -89,9 +89,9 @@ private void openFile(FileScanFramework.FileSchemaNegotiator negotiator) {
reader = new XMLReader(fsStream, dataLevel, maxRecords);
reader.open(rootRowWriter, errorContext);
} catch (Exception e) {
- throw UserException
+ throw UserException //lgtm[java/unused-format-argument]
Review comment:
yep, my bad, forgot to remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808863750
> The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with (via null guard or logger); however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-812876055
Yeah for sure I'll see what I can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805086618
Errors in CI involve drillbuf.java, will remove.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-846279367
I rebased successfully, when pushing to my original branch there were conflicts so I attempted to pull but then I had to resolve some merge conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805021053
As a side note: I had not touched Drillbuf.java, or Drillfilesystem.java, but upon making this I found them. I can remove them if they're unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-846097156
Hi @eevanwong
Could you please rebase with current master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808755262
@eevanwong I will check the questions in your comments tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808392405
The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with; however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-809546908
1. According to their website, LGTM takes the codebase, "generates a database to represent the codebase", then uses codeQL to run queries on the database to generate alerts. They use data science to identify the errors (from analyzing a lot of other projects and finding common errors).
2. It depends. Sometimes, alerts are a false-positive (incorrect); however, pertaining to drill, most of the alerts were meaningful as they were valid issues that needed to be fixed; however, some of them are not. For example,
This already has a guard if the counter is greater than the length of the data, despite that, it still throws the alert, and so I tried to use a try and catch statement.
Also, like I said in my previous comment. Sometimes the value is initialized in the null value as an arbitrary value so that it can be used later. Just declaring it isn't enough as it causes an error.
3. For hashcode, it recommends overriding both `hashCode` and `equals` methods to ensure they're consistent. The thing is, in most cases, there is no hashCode or equals method.
e.g
This results in this hashcode alert
For nulls, it recommends to ensure that the value is not null when its dereferenced.
The thing, the alert is essentially explaining that the value might be null b/c it was initialized as so, but thats because it has to be arbitrary initialized so that it can be called in different statements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-814914037
Hey so, a bit of an update, for hash code and equals, tuns out you in fact do need both.
> Yes, this is a legitimate concern. The contract between hashCode and equals is that equal instances must have the same hashCode. The reverse is not necessary, but high quality hashCode algorithms should avoid this as much as possible.
According to javadoc for the equals method:
Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.
Full javadoc is here: equals and hashCode.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vdiravka commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
vdiravka commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-945854772
Is this similar to recently merged: #2310
@estherbuchwalter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808321999
Hey, could I get someones input on this? This is quite a small thing, but still something I want to make sure.
This is in JsonTableGroupScan.java, and essentially the alert is that there were only 4 arg to support the 5 that the format call was requesting for. The thing is, I can't find a fn in indexDesc that deals with finding the estimatedSize. For now, I put in some default string, but I would like to know if I could potentially find estimated size, or I could just delete that portion in the format call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong closed pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong closed pull request #2191:
URL: https://github.com/apache/drill/pull/2191
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805268301
@eevanwong
Thank you for the submission. I see a lot of suppressions here. Are you going to fix some of these, or just suppress the warnings?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2191:
URL: https://github.com/apache/drill/pull/2191#discussion_r612090666
##########
File path: contrib/format-xml/src/main/java/org/apache/drill/exec/store/xml/XMLBatchReader.java
##########
@@ -89,9 +89,9 @@ private void openFile(FileScanFramework.FileSchemaNegotiator negotiator) {
reader = new XMLReader(fsStream, dataLevel, maxRecords);
reader.open(rootRowWriter, errorContext);
} catch (Exception e) {
- throw UserException
+ throw UserException //lgtm[java/unused-format-argument]
Review comment:
If you've fixed the cause of the lgtm warning, do we still need the comment?
##########
File path: contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaPartitionScanSpec.java
##########
@@ -22,7 +22,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-public class KafkaPartitionScanSpec {
+public class KafkaPartitionScanSpec { //lgtm[java/inconsistent-equals-and-hashcode]
Review comment:
We should probably fix this. Inconsistent equals and hashcode could probably result in weird errors.
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/NetworkFunctions.java
##########
@@ -431,7 +431,7 @@ public void eval() {
int power = 3 - i;
try {
int ip = Integer.parseInt(ipAddressInArray[i]);
- result += ip * Math.pow(256, power);
+ result += (long)(ip * Math.pow(256, power)); //lgtm[java/implicit-cast-in-compound-assignment]
Review comment:
Do we still need the comment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on a change in pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on a change in pull request #2191:
URL: https://github.com/apache/drill/pull/2191#discussion_r612097419
##########
File path: contrib/storage-kafka/src/main/java/org/apache/drill/exec/store/kafka/KafkaPartitionScanSpec.java
##########
@@ -22,7 +22,7 @@
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
-public class KafkaPartitionScanSpec {
+public class KafkaPartitionScanSpec { //lgtm[java/inconsistent-equals-and-hashcode]
Review comment:
Yeah definitely. I've been a bit split in terms of work so I'll look into this later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808862839
> Hey, could I get someones input on this? This is quite a small thing, but still something I want to make sure.
>
> 
>
> This is in JsonTableGroupScan.java, line 519. The alert is that there were only 4 arg to support the 5 that the format call was requesting for. The thing is, I can't find a fn in indexDesc that deals with finding the estimatedSize. For now, I put in some default string, but I would like to know if I could potentially find estimated size, or I could just delete that portion in the format call.
About all the alarms of the MapR module, you can ignore them directly (Maybe you only need to mark as comments in this PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808867026
> The most common issues that I suppressed involved null values and potential NPE and override hashcode and equals. The thing with the null values is that, most times in the codebase, if the value was null it would already be dealt with (via null guard or logger); however, it still marks it as an alert. With hashcode and equals, there were sometimes a need for only hashcode and not equals, and vice versa. If one or the other were not present in the file, it would throw an alert. Also, in some cases, the equals function was there, but it was not named equals (it was named impEquals), which LGTM prob did not pick up, and threw the alert.
I have noticed your question. Before resolve, I would like to ask you to research the following:
1. How does LGTM.com work?
2. Are all alarms meaningful on LGTM.com (Help focus your mind)?
3. Could you please explain what is LGTM's suggestions for handle the issues (NPE, hashcode)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-812024522
1. Yes there are 17 alerts in total caused by this issue.
2. I'm not sure if the null errors are related, the main issue on the alert was that it would point to an NPE if its value didnt change by the time it got to a statement that would use it.
I suppose I could try ensuring var is not null forever; however, it would just be towards another arbitrary value and it would be odd to do it depending on what type of variable it is (e.g File variable I wouldnt know what value to point it towards).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-817156754
1. Yes, there is still a few changes I can make before moving onto the uncertain ones.
2. Yeah I'll work on what I can for now (altho I'm a little busy during the weekend so it'll have to wait)
3. I agree the overview is a bit messy, I'll reorganize it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vdiravka edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
vdiravka edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-945854772
Is this similar to recently merged: #2310 ?
@estherbuchwalter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805346188
Hey, yeah there are a lot of suppressions. The next step for me is to fully understand all the ??? ones, then determining if I can fix. I just wanted to make a pr now so I didnt have to do it later.
I do think that I would keep certain ones suppressed regardless, specifically the documentation ones, as that has no particular impact on the actual code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-814914037
Hey so, a bit of an update, for hash code and equals, tuns out you in fact do need both.
> Yes, this is a legitimate concern. The contract between hashCode and equals is that equal instances must have the same hashCode. The reverse is not necessary, but high quality hashCode algorithms should avoid this as much as possible.
According to javadoc for the equals method:
Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.
https://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#equals(java.lang.Object)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-846278755
Sorry, been awhile, I got hard sidetracked by school and whatnot. That being said I think I screwed up the rebase. :/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805346188
Hey, yeah there are a lot of suppressions. The next step for me is to fully understand all the ??? ones, then determining if I can fix. I just wanted to make a pr now so I didnt have to do it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on a change in pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on a change in pull request #2191:
URL: https://github.com/apache/drill/pull/2191#discussion_r612096847
##########
File path: contrib/udfs/src/main/java/org/apache/drill/exec/udfs/NetworkFunctions.java
##########
@@ -431,7 +431,7 @@ public void eval() {
int power = 3 - i;
try {
int ip = Integer.parseInt(ipAddressInArray[i]);
- result += ip * Math.pow(256, power);
+ result += (long)(ip * Math.pow(256, power)); //lgtm[java/implicit-cast-in-compound-assignment]
Review comment:
Nope. Will change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-805021053
As a side note: I had not touched Drillbuf.java, or Drillfilesystem.java, but upon making this I found that these changes. I can remove them if they're unnecessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong edited a comment on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong edited a comment on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-808321999
Hey, could I get someones input on this? This is quite a small thing, but still something I want to make sure.
This is in JsonTableGroupScan.java, line 519. The alert is that there were only 4 arg to support the 5 that the format call was requesting for. The thing is, I can't find a fn in indexDesc that deals with finding the estimatedSize. For now, I put in some default string, but I would like to know if I could potentially find estimated size, or I could just delete that portion in the format call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] eevanwong commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
eevanwong commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-846285854
Ok, I think it worked
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] estherbuchwalter commented on pull request #2191: Drill 7882 + Drill 7883 - Fix LGTM Alerts in /common and /contrib
Posted by GitBox <gi...@apache.org>.
estherbuchwalter commented on pull request #2191:
URL: https://github.com/apache/drill/pull/2191#issuecomment-945893420
I don't think that this overlaps with recently merged #2310. #2310 focused on adding Javadoc param tag descriptions and fixing Javadoc related errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org