asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Blow (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Enabled Feed Tests and Added External Library tests
Date Mon, 15 Feb 2016 16:09:01 GMT
Michael Blow has posted comments on this change.

Change subject: Enabled Feed Tests and Added External Library tests
......................................................................


Patch Set 12:

(77 comments)

https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-app/data/external-parser/dropbox/jobads1.txt
File asterix-app/data/external-parser/dropbox/jobads1.txt:

Line 3:         BlockWrites = 0; 
trailing whitespace?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java
File asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java:

Line 601:                         case "lib": // expected format <dataverse-name> <library-name>
<library-directory>
Space is not a legal character for any of dataverse, library name, library directory?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java
File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java:

Line 74:             th.printStackTrace();
Is this temporary or accidental?  Should we have a TODO to remove?


Line 85:             th.printStackTrace();
Is this temporary or accidental?  Should we have a TODO to remove?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java
File asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java:

Line 284:                         //mBuffer.sendReport(frame);
Why is this commented out?  Is it no longer applicable (i.e. should we just remove it?)


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java
File asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java:

Line 63:      * @param feedLogFileSplits 
trailing whitespace (and next line)


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java
File asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java:

Line 77:                             "Unspecified (\"reader\" or \"format\") parameter for
local file system adapter");
Prefer "filesystem" myself, but I guess both are acceptable.

Do exactly one of these need to be specified?  Should this error message be clarified to indicate
what is expected?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java
File asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java:

Line 142:         recordLogger.newLine();
Each call to recordLogger is pretty heavy, includes synchronization, using StringBuilder and
calling a single recordLogger.write(buf.toString()) would be preferred...


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java:

Line 130:         System.arraycopy(otherString.toCharArray(), 0, value, 0, length);
Should use String.getChars() to avoid the temporary char [] allocation (toCharArray())


Line 336:         System.arraycopy(aString.toCharArray(), 0, value, i, aString.length());
Should use String.getChars() to avoid the temporary char [] allocation (toCharArray())


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/AttributeReference.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/AttributeReference.java:

Line 78:      * 
trailing ws


Line 90:      * 
trailing ws


Line 113:      * 
trailing ws


Line 132:                 // Will this check result in infinite recursion? How do I stop it?

trailing ws


Line 234:         // lookup with scope; this may side-affect state        
trailing ws


Line 402:      * 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/BuiltinClassAdFunctions.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/BuiltinClassAdFunctions.java:

Line 58:             if (name.equalsIgnoreCase("isundefined")) {
equalsIgnoreCase is not very efficient when used over and over I think- replacing this with
a switch on name.toLowerCase() would probably be better.


Line 109:             // undefined and we're supposed to test for strict comparison, the 
trailing ws


Line 234:                 // Either take the number if it's the first, 
trailing ws


Line 305:                 // For this element of the list, make sure it is 
trailing ws


Line 363:             } else if (!stringValue.isStringValue(comparison_string)) {
This seems easy to miss that this "is" test method has the side-effect of copying the string
value into comparison_string.  Perhaps we could change this to setIfStringValue() or something?
 Dunno.


Line 369:             if (comparison_string.equalsString("<")) {
Seems a good candidate for a string-switch.


Line 422:                 // For this element of the list, make sure it is 
trailing ws


Line 581:         format = format.replaceAll("%m", "MM");
You don't want replaceAll here, that is denoting a regex, I think you're trying to do 1:1
replacement, which would be best serviced by String.replace().


Line 620:                 if (name.equalsIgnoreCase("getyear")) {
Same comment as above, I would favor string switch on case-normalized name


Line 646:                 if (name.equalsIgnoreCase("getyear") || name.equalsIgnoreCase("getmonth")
Same comment as above, I would favor string switch on case-normalized name


Line 878:             // only one argument 
trailing ws


Line 1192:                 // use the printpretty on arg0 to spew out 
trailing ws


Line 1288:                 // absTime with no arguments returns the current time. 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/CaseInsensitiveString.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/CaseInsensitiveString.java:

Line 47:         return 0;
Shouldn't we implement this, comparing the normalized (lower case) value?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAd.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAd.java:

Line 466:         // only try to process if the string is valid 
trailing ws


Line 499:                 // if caching is enabled, and we got to here then we know that the

trailing ws


Line 617:                     return EvalResult.EVAL_FAIL.ordinal(); // NAC 
trailing ws


Line 639:     // --- begin deletion methods 
trailing ws


Line 717:         // already set by base class for this node; we shouldn't propagate 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAdUnParser.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/ClassAdUnParser.java:

Line 53:      * 
trailing ws


Line 133:                     // digits as possible, which is why we don't use the 
trailing ws


Line 199:      * 
trailing ws


Line 473:      * based on the character content, 
trailing ws


Line 474:      * it's unparsed either as a quoted attribute or non-quoted attribute 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/ExprTree.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/ExprTree.java:

Line 38:         /// Attribute reference node (attr, .attr, expr.attr) 
trailing ws


Line 42:         /// Function call node 
trailing ws


Line 44:         /// ClassAd node 
trailing ws


Line 46:         /// Expression list node 
trailing ws


Line 130:      * 
trailing ws


Line 139:      * 
trailing ws


Line 148:      * 
trailing ws


Line 301:      * 
trailing ws


Line 320:      * 
trailing ws


Line 327:      * 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/FunctionCall.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/FunctionCall.java:

Line 108:         // pattern matching (regular expressions) 
trailing ws


Line 120:         // turn the contents of an expression into a string 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/InputStreamLexerSource.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/InputStreamLexerSource.java:

Line 77:             System.arraycopy(buffer, 0, buffer, 1, buffer.length - 1);
Doesn't this instead duplicate the next character instead of reinstating the last one?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Lexer.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/Lexer.java:

Line 580:     // tokenizeStringLiteral:  Scans strings of the form " ... " or '...' 
trailing ws


Line 642:     // tokenizePunctOperator:  Tokenize puncutation and operators 
trailing ws


Line 840:         // cut the token and return 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/LexerSource.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/LexerSource.java:

Line 31:      * LexerSource that provide access to specific types of sources. 
trailing ws


Line 53:     // ever put back a single character. 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Literal.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/Literal.java:

Line 115:     /* Creates an absolute time literal, from the string timestr, 
trailing ws


Line 215:     /* Creates a relative time literal, from the string timestr, 
trailing ws


Line 319:     /* Function which iterates through the string Str from the location 'index',

trailing ws


Line 320:      *returning the index of the next digit-char 
trailing ws


Line 366:      *  which is the number of seconds since the epoch 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Operation.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/Operation.java:

Line 41:     /// List of supported operators 
trailing ws


Line 110:      * 
trailing ws


Line 133:      * 
trailing ws


Line 147:      * 
trailing ws


Line 163:      * 
trailing ws


Line 674:         // if op is binary, but not associative or commutative, disallow splitting

trailing ws


Line 927:                     // comparison between strings and non-exceptional non-string

trailing ws


Line 1218:         // trap sigfpe and set the ClassAdExprFPE flag to true; on NT check the

trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Util.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/Util.java:

Line 74:                                 char digit = text.charAt(source + 1); // is the next
digit also 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/Value.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/Value.java:

Line 85:     public boolean isBooleanValue(MutableBoolean b) {
Same comment as earlier- personally I would prefer some method naming that would indicate
there is a side-effect of the 'is' check (i.e. setting the value of the passed boolean holder)


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/object/pool/CharArrayStringPool.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/object/pool/CharArrayStringPool.java:

Line 27:         return new AMutableCharArrayString(32);
Why 32 for pool instances?


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdParserTest.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdParserTest.java:

Line 51:      * 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdToADMTest.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdToADMTest.java:

Line 60:      * 
trailing ws


https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdUnitTester.java
File asterix-external-data/src/test/java/org/apache/asterix/external/classad/test/ClassAdUnitTester.java:

Line 85:             // Then we parse to see what the user wants. 
trailing ws


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/625
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idd1fccd136fa2645b2707bbf7c04e60991ae8d4a
Gerrit-PatchSet: 12
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <michael.blow@couchbase.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-HasComments: Yes

Mime
View raw message