Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12481 )
Change subject: IMPALA-7368: Add initial support for DATE type ...................................................................... Patch Set 3: (8 comments) I did a pass over the remaining bits of the patch. Overall it looks really good. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.h File be/src/runtime/date-value.h: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/runtime/date-value.h@59 PS6, Line 59: DateValue(int32_t days_since_epoch) : days_since_epoch_(INVALID_DAYS_SINCE_EPOCH) { Can you make these constructors explicit? Just to avoid any potential unexpected conversions. http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/12481/6/be/src/service/hs2-util.cc@539 PS6, Line 539: RawValue::PrintValue(value, TYPE_DATE, -1, &(hs2_col_val->stringVal.value)); > Can we just use StringValue::ToString() instead? Seems like a lot of indire I still think it would be good to fix this, just so that the new code is efficient. http://gerrit.cloudera.org:8080/#/c/12481/6/fe/src/main/java/org/apache/impala/analysis/DateLiteral.java File fe/src/main/java/org/apache/impala/analysis/DateLiteral.java: http://gerrit.cloudera.org:8080/#/c/12481/6/fe/src/main/java/org/apache/impala/analysis/DateLiteral.java@40 PS6, Line 40: strValue_ = strValue; Mention that it's the same representation as DateValue in backend? http://gerrit.cloudera.org:8080/#/c/12481/6/fe/src/main/java/org/apache/impala/analysis/DateLiteral.java@127 PS6, Line 127: Why not compare daysSinceEpoch_? http://gerrit.cloudera.org:8080/#/c/12481/6/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java File fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java: http://gerrit.cloudera.org:8080/#/c/12481/6/fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java@81 PS6, Line 81: alysis whether http://gerrit.cloudera.org:8080/#/c/12481/6/testdata/workloads/functional-query/queries/QueryTest/compute-stats-date.test File testdata/workloads/functional-query/queries/QueryTest/compute-stats-date.test: http://gerrit.cloudera.org:8080/#/c/12481/6/testdata/workloads/functional-query/queries/QueryTest/compute-stats-date.test@4 PS6, Line 4: create table date_tbl like functional.date_tbl; Test might be slightly clearer if this had a different name from the functional version, e.g. tmp_date_tbl or date_tbl_copy. http://gerrit.cloudera.org:8080/#/c/12481/6/testdata/workloads/functional-query/queries/QueryTest/date.test File testdata/workloads/functional-query/queries/QueryTest/date.test: http://gerrit.cloudera.org:8080/#/c/12481/6/testdata/workloads/functional-query/queries/QueryTest/date.test@35 PS6, Line 35: select count(*) from date_tbl We should test the IN predicate with both long and short lists to exercise both the set lookup and iterate code. I don't expect there to be bugs here but it's just interesting enough that it would be good to have the coverage. // Threshold based on InPredicateBenchmark results int setLookupThreshold = children_.get(0).getType().isStringType() ? 2 : 6; if (children_.size() - 1 < setLookupThreshold) useSetLookup = false; http://gerrit.cloudera.org:8080/#/c/12481/6/testdata/workloads/functional-query/queries/QueryTest/date.test@36 PS6, Line 36: where '2017-11-28' in (date_col) Is it deliberate that all this IN/NOT IN/BETWEEN predicates can be rewritten to simple comparisons by the planner? I don't understand focusing on this rather than variants that can't be simplified (testing edge cases is good, we just need to test the regular cases too). -- To view, visit http://gerrit.cloudera.org:8080/12481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iea8155ef09557e0afa2f8b2d0b2dc9d0896dc30f Gerrit-Change-Number: 12481 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 05 Mar 2019 02:43:28 +0000 Gerrit-HasComments: Yes
