Michael Brown has posted comments on this change. Change subject: IMPALA-3718: Support subset of functional-query for Kudu ......................................................................
Patch Set 1: (6 comments) In addition to the inline comments, I generally disagree on the surface with the use of xfail. It seems like many or all of them should be skips. Any reason you chose xfail? Also, have you tested this end to end? Some ideas: 1. ./buildall.sh -format -testdata [etc] 2. Private data load job 3. ./buildall.sh -format -snapshot_file snapshot_from_private_job -metastore_snapshot_file metastore_snapshot_from_private_job [etc] http://gerrit.cloudera.org:8080/#/c/4175/1/testdata/bin/generate-schema-statements.py File testdata/bin/generate-schema-statements.py: Line 594: print "Ignore partitions on Kudu" Include the db and table name here so we know what we're ignoring. http://gerrit.cloudera.org:8080/#/c/4175/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: PS1, Line 598: SELECT row_number() over (order by year, month, id, day), For my education, was the ordering of "id, day" in the ORDER BY intentional? Why that instead of "day, id"? PS1, Line 1063: text_comma_backslash_newline This table is defined as a kudu table in schema_constraints.csv L181, but it doesn't have a CREATE_KUDU section. Does it need one? http://gerrit.cloudera.org:8080/#/c/4175/1/testdata/datasets/functional/schema_constraints.csv File testdata/datasets/functional/schema_constraints.csv: PS1, Line 177: table_name:testtbl, constraint:only, table_format:kudu/none/none How is it that this constraint was already defined but only just now had a CREATE_KUDU added for it (functional_schema_template.sql, L789 )? http://gerrit.cloudera.org:8080/#/c/4175/1/testdata/workloads/functional-query/queries/QueryTest/aggregation.test File testdata/workloads/functional-query/queries/QueryTest/aggregation.test: Line 836: select min(distinct NULL), max(distinct NULL) from alltypes Why this change? Thanks. http://gerrit.cloudera.org:8080/#/c/4175/1/testdata/workloads/functional-query/queries/QueryTest/create_kudu.test File testdata/workloads/functional-query/queries/QueryTest/create_kudu.test: PS1, Line 4: drop table if exists managed_kudu; Fwiw, this won't be needed once the commit lies atop https://gerrit.cloudera.org/#/c/4155/ -- To view, visit http://gerrit.cloudera.org:8080/4175 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iada88e078352e4462745d9a9a1b5111260d21acc Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
