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 <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-HasComments: Yes
