Skip to content

Fix SingleStore remote query#12698

Closed
wendigo wants to merge 2 commits intotrinodb:masterfrom
wendigo:serafin/singlestore-fix
Closed

Fix SingleStore remote query#12698
wendigo wants to merge 2 commits intotrinodb:masterfrom
wendigo:serafin/singlestore-fix

Conversation

@wendigo
Copy link
Copy Markdown
Contributor

@wendigo wendigo commented Jun 6, 2022

Replaces #12696

All tests are passing locally

@cla-bot cla-bot bot added the cla-signed label Jun 6, 2022
@wendigo wendigo requested review from ebyhr, kokosing and ssheikin June 6, 2022 10:58
@wendigo wendigo force-pushed the serafin/singlestore-fix branch from ad12c53 to 5db4849 Compare June 6, 2022 11:06
Remote query cannot be constructed with empty columns list
@wendigo wendigo force-pushed the serafin/singlestore-fix branch from 5db4849 to a98ca5d Compare June 6, 2022 11:23
ResultSetMetaData metadata = preparedStatement.getMetaData();
if (metadata == null) {
// SingleStore returns empty metadata instead of a null value
if (metadata == null || metadata.getColumnCount() == 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if this change is acceptable or not since existing MariaDB failure case depends on this block.
(That is why I was going to fix the test message in #12696)

cc: @kasiafi @findepi

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As I've stated in a commit message: constructing Query PTF without columns doesn't make sense any way so this condition seems fine to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method is supposed to create a table handle for a PreparedQuery. Apparently, it is able to create a valid TableHandle with no columns. Let the caller take responsibility of handling the "no columns" condition. A TableHandle without columns might make no sense for a Table Function, but it might be OK in some other context (in the future). If a TableHandle with no columns is never OK, then maybe we miss a check in the JdbcTableHandle constructor?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can relation have no columns?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A relation should have at least one column (though there exist sub-plans with no columns as a result of optimization).

My point is that a TH with no columns could still be potentially useful if it represents e.g. an insert. Adding the column on Trino side would not be a big issue.

@ebyhr ebyhr requested review from findepi and kasiafi June 6, 2022 11:58
@wendigo wendigo closed this Jun 28, 2022
@wendigo wendigo deleted the serafin/singlestore-fix branch June 28, 2022 11:11
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.

3 participants