[Impala-ASF-CR] IMPALA-3718: Support subset of functional-query for Kudu

["Matthew Jacobs (Code Review)" <ge...@cloudera.org>]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3718: Support subset of functional-query for Kudu
......................................................................


Patch Set 1:

(8 comments)

Thanks for the feedback.

My thought with xfail was that I wanted them to show up as xfail so we see them and have an incentive to fix them in the future. We should be able to add support for the remaining tables once kudu addresses the JIRAs I mentioned in the commit comment. I can change them to skips if you still prefer.

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.
Done


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
It doesn't really matter too much. This row_number() column gets hidden anyway. If anything I might have put id first. There is only 1 distinct year and 1 distinct month, so they don't change the order.


PS1, Line 1063: text_comma_backslash_newline
> This table is defined as a kudu table in schema_constraints.csv L181, but i
Good catch it shouldn't be there. Thanks.


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 
generate_schema_statements will generate a create table statement if it's not defined (using the first col as the PK and hash expr), but I wanted to have them explicitly defined.


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.
IMPALA-4042
alltypesagg is a view for kudu so this wouldn't work. The test still exercises the target functionality on a different table.


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.clouder
Thanks! Removing...


http://gerrit.cloudera.org:8080/#/c/4175/1/tests/common/test_result_verifier.py
File tests/common/test_result_verifier.py:

PS1, Line 333:     # TINYINT). Bypass the type # checking by ignoring the actual types of the Avro
> Nit: You left the # when joining the line.
Done


PS1, Line 336:       if 'TIMESTAMP' in expected_types:
             :         LOG.info("TIMESTAMP columns unsupported in %s, skipping verification." %\
             :             file_format)
             :         return
> It's weird to see this logic again.
this can be removed, thanks


-- 
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 <mj...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes