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.
Line 594: print "Ignore partitions on Kudu"
> Include the db and table name here so we know what we're ignoring.
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.
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.
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.
PS1, Line 4: drop table if exists managed_kudu;
> Fwiw, this won't be needed once the commit lies atop https://gerrit.clouder
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:
: LOG.info("TIMESTAMP columns unsupported in %s, skipping
> 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-Owner: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>