Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17706 )
Change subject: IMPALA-3430: Runtime filter : Extend runtime filter to support Min/Max values for HDFS scans ...................................................................... Patch Set 25: (14 comments) Thanks for applying the changes. I mainly reviewed the string +1/-1 part this time. http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.h File be/src/runtime/string-value.h: http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.h@130 PS25, Line 130: std::string LeastSmallerString() const; please add some backend tests for these functions http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc File be/src/runtime/string-value.cc: http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@49 PS25, Line 49: string() nit: could be just return ""; or return {}; http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@52 PS25, Line 52: while (i >= 0 && ptr[i] == 0x00) { : i--; : } nit: could fit in a single line http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@55 PS25, Line 55: i == -1 probably add UNLIKELY? http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@56 PS25, Line 56: 0xff chars. Shouldn't be 0x00 chars if we want a string smaller than '*this'? http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@62 PS25, Line 62: string result(ptr, i); : // append a char which is ptr[i]-1 : result.append(1, (uint8_t)(ptr[i]) - 1); : // copy all remaining characters (which must be 0x00) in [i+1, size()-1] : result.append(ptr + i + 1, len - i - 1); The appends might need to allocate a bigger buffer for the string. We could pre-allocate the buffer with the required size: string result; result.reserve(len); result.append(ptr, i); ... http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@75 PS25, Line 75: i == -1) probably add UNLIKELY? http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@81 PS25, Line 81: len+1 0x00 chars Shouldn't be len+1 0xFF if we want a larger string? Or (len 0xFF) + 0x00 if we want the least larger string? http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@87 PS25, Line 87: string result(ptr, i); Same as above. I think we should reserve the required amount of space before appending. http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/runtime/string-value.cc@89 PS25, Line 89: result.append(1, (uint8_t)(ptr[i]) + 1); At this point result is already larger then *this. I don't think that we need to copy the remaining parts of the string. http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/util/min-max-filter-test.cc File be/src/util/min-max-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/util/min-max-filter-test.cc@572 PS25, Line 572: two0xffChars It should be the least smaller string of threeNullVal, but two0xffChars is greater than threeNullVal. http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/util/min-max-filter.h File be/src/util/min-max-filter.h: http://gerrit.cloudera.org:8080/#/c/17706/25/be/src/util/min-max-filter.h@248 PS25, Line 248: a string of 1 byte of 0x0. shouldn't be the empty string? http://gerrit.cloudera.org:8080/#/c/17706/25/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java File fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java: http://gerrit.cloudera.org:8080/#/c/17706/25/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@457 PS25, Line 457: getIsNonCorrelatedScalarSubquery nit: maybe just isNonCorrelatedScalarSubquery()? http://gerrit.cloudera.org:8080/#/c/17706/25/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/17706/25/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@641 PS25, Line 641: getIsNonCorrelatedScalarSubquery nit: isNonCorrelatedScalarSubquery()? -- To view, visit http://gerrit.cloudera.org:8080/17706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c2bb5baad622051d1002c9c162c672d428e5446 Gerrit-Change-Number: 17706 Gerrit-PatchSet: 25 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 11 Aug 2021 11:25:29 +0000 Gerrit-HasComments: Yes
