impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3718: Support subset of functional-query for Kudu
Date Tue, 13 Sep 2016 20:25:59 GMT
Matthew Jacobs has posted comments on this change.

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

Patch Set 1:


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.
File testdata/bin/

Line 594:         print "Ignore partitions on Kudu"
> Include the db and table name here so we know what we're ignoring.
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.
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.
File testdata/workloads/functional-query/queries/QueryTest/aggregation.test:

Line 836: select min(distinct NULL), max(distinct NULL) from alltypes
> Why this change? Thanks.
alltypesagg is a view for kudu so this wouldn't work. The test still exercises the target
functionality on a different table.
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...
File tests/common/

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

PS1, Line 336:       if 'TIMESTAMP' in expected_types:
             :"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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iada88e078352e4462745d9a9a1b5111260d21acc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-HasComments: Yes

View raw message