Fix failure when query table function contains column alias in JDBC connectors

[ebyhr]

Description

Fixes #16225

Release notes

(x) Release notes are required, with the following suggested text:

# MySQL, Druid, MariaDB, SingleStore
* Fix failure when `query` table function contains column alias. ({issue}`16225`)

[martint]

There's no guarantee that the outputs of the query will match the column names in the table function. If you want to be absolutely sure, you should do something like:

SELECT region_name FROM TABLE(...)

which should fail if the column is not exposed by the table function.

[ebyhr]

Changing to the suggested query doesn't guarantee either and it hides the actual output column names. For instance, the below test pass in H2 because the generated query will be SELECT "REGION_NAME" FROM (SELECT name AS region_name FROM tpch.region WHERE regionkey = 0) o.

        assertThat(query(format("SELECT region_name FROM TABLE(system.query(query => 'SELECT name AS region_name FROM %s.region WHERE regionkey = 0'))", getSession().getSchema().orElseThrow())))
                .skippingTypesCheck()
                .hasOutputNames(ImmutableList.of("region_name"))
                .matches("VALUES 'AFRICA'");

[martint]

Not sure what you mean. If the table function doesn’t expose the name, the outer query should fail with a column not found error.

Also, what do you mean by “passes on H2”? That query should not even run against H2, since it H2 doesn’t support table functions, or any of the table functions in Trino.

[martint]

For example, given:

SELECT region_name FROM TABLE(system.query(query => 'SELECT name FROM tpch.region WHERE regionkey = 0'));

=>

Query 20230225_223332_00030_tgtxv failed: line 1:8: Column 'region_name' cannot be resolved

[ebyhr]

We have TestJdbcConnectorTest running on H2.

The given example ensures column names without case sensitivity. The below query (added a column alias to the table function) should pass in my understanding because SELECT name FROM (SELECT name as "NAME" FROM region) is a valid query in Trino. This is what happening on the above H2 example, the table function exposes the column names with uppercase, but adding the projection lowercase the final output column names. That's why I didn't add the projection.

SELECT name FROM TABLE(system.query(query => 'SELECT name AS "NAME" FROM tpch.region WHERE regionkey = 0'));
  name
--------
 AFRICA

[martint]

We have TestJdbcConnectorTest running on H2.

Some tests do, but QueryAssert does not use H2. It runs the given query and compares the results to the output of the .matches() clause (which is another query that runs in Trino).

[martint]

The given example ensures column names without case sensitivity.

Well, it tests that the name is resolvable using Trino's identifier resolution rules (which, unfortunately, currently don't follow proper SQL semantics).

[ebyhr]

Some tests do, but QueryAssert does not use H2. It runs the given query and compares the results to the output of the .matches() clause (which is another query that runs in Trino).

TestJdbcConnectorTest uses createH2QueryRunner. The behind database is H2 regardless of assertion methods.

[ebyhr]

As I replied in Slack, SELECT name FROM ... ensures the internal flow, but it doesn't ensure the final output. It would be nice to verify the output column names as well. Otherwise, we may miss the future change about case sensitivity.

[ebyhr]

@martint Gentle reminder.

[ebyhr]

@martint Please take a look at the above comments.

[ebyhr]

@martint Please take another look.

[wendigo]

LGTM! Thanks @ebyhr

[ebyhr]

@hashhar @Praveen2112 Gentle reminder.

[hashhar]

Looks good to me. @Praveen2112 do you want to take a look?

I don't understand Martin's concerns since none of this happens within the engine, it's how the connector reports information back to the engine and this is actually a bugfix showcased by the tests.

[martint]

What I said above is that we shouldn’t be verifying that the table function exposes the expected names by checking how they are propagated outside of the outer query. We can validate that by having the outer query try to resolve the names directly.

[ebyhr]

We can validate that by having the outer query try to resolve the names directly.

testNativeQueryColumnAlias covers this scenario.

Stale.