Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRILL-7968: ANALYZE TABLE ... REFRESH METADATA fails with FLOAT4 column. #2267

Merged
merged 2 commits into from
Jul 7, 2021

Conversation

jnturton
Copy link
Contributor

@jnturton jnturton commented Jul 6, 2021

This commit adds support for java.lang.Float to PojoWriters.java and a
test for the problem described in DRILL-7968 to TestMetastoreCommands.java.

For tables with fewer than ~200 rows and a FLOAT4 column there is no bug: the ANALYZE command succeeds and, indeed, this case is exercised by tests in TestMetastoreCommands.java and alltypes_{required,optional}.parquet which both contain a FLOAT4 column.

But for tables with more than ~200 rows and a FLOAT4 column the ANALYZE command fails with

SQL Error: EXECUTION_ERROR ERROR: PojoRecordReader doesn't yet support conversions from the type [class java.lang.Float].

Failed to setup reader: DynamicPojoRecordReader

E.g. you can reproduce the above with

create table dfs.tmp.test_analyze as
select cast(1 as float) from cp.`employee.json`;

analyze table dfs.tmp.test_analyze refresh metadata;

Documentation

No user-visible change.

Testing

TestMetastoreCommands.java and ad hoc SQL testing.

This commit adds support for java.lang.Float to PojoWriters.java and a
test for the problem described in DRILL-7968 to TestMetastoreCommands.java.
Copy link
Member

@luocooong luocooong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dzamo Nice fix. +1
We can quickly merge this pull request once all the unit tests passed.

@@ -3531,6 +3531,40 @@ public void testAnalyzeWithNonWritableWorkspace() throws Exception {
run("analyze table dfs.%s.%s refresh metadata", workspaceName, tableName);
}

@Test
public void testAnalyzeAllTypes7kRows() throws Exception {
// See DRILL-7968. More rows are produced here to test an ANALYZE code path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be shortened to :

@Test // DRILL-7968

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I've applied the fixes and a build+test run should kick off presently.

@jnturton
Copy link
Contributor Author

jnturton commented Jul 7, 2021

@luocooong I think all the builds have passed at least once now. There is one cancelled main build reported under Checks. Do I need to do any builds here?

@luocooong
Copy link
Member

@dzamo It's time to merge. Squash all the commits for better.

@jnturton jnturton merged commit 14ba6a8 into apache:master Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants