[CALCITE-7642] RelToSqlConverter may generate duplicate aliases for internal derived relations in case-insensitive dialects#5077
Conversation
… names for case-insensitive dialects
|
I added a comment in jira. |
@xiedeyantu Thanks for reviewing. I replied on the JIRA issue with a more precise description. |
… tables in case-insensitive dialects
zzwqqq
changed the title
| } | ||
|
|
||
| /** Returns field names to use when a result is wrapped as a derived table. */ | ||
| protected List<String> derivedTableFieldNames(RelDataType rowType) { |
There was a problem hiding this comment.
Is this function really useful?
There was a problem hiding this comment.
Thanks for reviewing. I removed it. SqlImplementor already has access to the dialect, so the extra hook was not needed. forDerivedTable now uniquifies the field names directly.
| } | ||
|
|
||
| /** Returns a copy of a row type with different field names. */ | ||
| private static RelDataType renameRowTypeFields(RelDataType rowType, |
There was a problem hiding this comment.
I wonder whether this is the right place for this function. It looks like it would belong as a method to RelRecordType.
There was a problem hiding this comment.
I considered moving it, but it is currently only used by SqlImplementor to track rewritten field aliases. I kept it local to avoid adding a RelRecordType method for a single use, but I can move it if you prefer.
| return aliasContext(aliases, true); | ||
| } | ||
|
|
||
| /** Returns a result that uses derived-table field names. */ |
There was a problem hiding this comment.
I am not sure the term "derived table" is defined in Calcite, people may wonder what it means.
Maybe you can add a description in the JavaDoc explaining when a table is derived.
There was a problem hiding this comment.
Thanks. I add clearer dosc to describe the "derived table".
| ignoreClauses, expectedClauses, expectedRel, forceExplicitAlias); | ||
| } | ||
|
|
||
| /** Rewrites a result for use in a derived table. */ |
There was a problem hiding this comment.
This doc does not describe the parameters, and is not very clear, maybe it will become clearer once you define derived tables.
There was a problem hiding this comment.
Thanks. I add clearer dosc.
| return withSelectFieldNames(fieldNames); | ||
| } | ||
|
|
||
| /** Rewrites a SELECT list to expose the given field names. */ |
There was a problem hiding this comment.
expose?
use the supplied names from {@code fieldNames}?
I suspect the count must match, do you need an assertion someplace about this?
There was a problem hiding this comment.
Thanks. I add clearer docs and assertion.
| } | ||
|
|
||
| /** Returns a result that uses derived-table field names. */ | ||
| /** Returns a result for use as a derived table, that is, a query in the |
There was a problem hiding this comment.
Let's then not call it a "table", tables are persistent. Call it a "relation" if you need. Or perhaps "fromDataSource", or something like that.
There was a problem hiding this comment.
Thanks. I renamed it to forDerivedRelation and updated the related docs to use “derived relation.” I updated the pr title and description too.
|
zzwqqq
changed the title
3 participants
Jira Link
CALCITE-7642
Changes Proposed
RelToSqlConverter may generate duplicate field aliases for internal derived relations in case-insensitive dialects.
A row type can contain field names such as
idandID. These names should be preserved as final output names, but when RelToSqlConverter wraps such a result as an internal derived relation, using those names directly can make later references ambiguous for a case-insensitive dialect.This patch uniquifies field aliases generated for internal derived relations using the target dialect's case-sensitivity. If an internal alias changes, the outer SELECT restores the original row type field name, for example
ID0 AS ID.Adds regression tests for root output preservation, derived values, joins, and correlate paths.