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

Reconstruct NOT IN when pushing down to JDBC source #6343

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

findepi
Copy link
Member

@findepi findepi commented Dec 15, 2020

Fixes #6075

@findepi findepi requested review from losipiuk and kokosing December 15, 2020 09:51
@cla-bot cla-bot bot added the cla-signed label Dec 15, 2020
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comments

"AND ((\"col_7\" >= ? AND \"col_7\" < ?) OR (\"col_7\" >= ? AND \"col_7\" < ?)) " +
"AND ((\"col_8\" >= ? AND \"col_8\" < ?) OR (\"col_8\" >= ? AND \"col_8\" <= ?)) " +
"AND ((\"col_9\" < ?) OR \"col_9\" IN (?,?)) " +
"AND (\"col_2\" = ?)");
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this assertion it is very detailed and so it it will be fragile to changes

Copy link
Member Author

Choose a reason for hiding this comment

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

It is required to visualize the changes and this is the only place where we test queries output to remote system.
Also, the structure of output queries changes very very rarely, so expected maint cost is low.

@@ -31,10 +33,13 @@ private Utils() {}
// just for this use case.
static final TypeOperators TUPLE_DOMAIN_TYPE_OPERATORS = new TypeOperators();

public static Block nativeValueToBlock(Type type, Object object)
public static Block nativeValueToBlock(Type type, @Nullable Object object)
Copy link
Member

Choose a reason for hiding this comment

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

Optional (separate commit)?

@@ -290,8 +290,8 @@ public void testBuildSqlWithDomainComplement()
assertThat(lastQuery).isEqualTo("" +
Copy link
Member

Choose a reason for hiding this comment

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

Do we have query in ATDQ to trigger this pushdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

@findepi
Copy link
Member Author

findepi commented Dec 15, 2020

CI #6266 (#5172?) and PT hang.

@findepi
Copy link
Member Author

findepi commented Dec 15, 2020

AC

@findepi findepi merged commit 6ad3369 into trinodb:master Dec 15, 2020
@findepi findepi deleted the findepi/jdbc/not/in branch December 15, 2020 12:39
@findepi findepi added this to the 349 milestone Dec 15, 2020
@findepi findepi mentioned this pull request Dec 28, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Preserve NOT IN when pushing down to JDBC source
3 participants