-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Multi-join pushdown appends recursively _<number> to column names so that they exceed maximum alias length #18642
Changes from all commits
e36d9f5
8fce268
25ea98e
cf29b3c
356a829
4346680
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
/* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package io.trino.plugin.jdbc; | ||
|
||
import io.trino.spi.connector.ConnectorSession; | ||
|
||
import static com.google.common.base.Splitter.fixedLength; | ||
import static com.google.common.base.Strings.padStart; | ||
|
||
public class ColumnWithAliasFormatter | ||
{ | ||
public static final int DEFAULT_COLUMN_ALIAS_LENGTH = 30; | ||
public static final int ORIGINAL_COLUMN_NAME_LENGTH = 24; | ||
|
||
public JdbcColumnHandle format(ConnectorSession session, JdbcColumnHandle column, int nextSyntheticColumnId) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ConnectorSession seems unused. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe name the class |
||
{ | ||
int sequentialNumberLength = DEFAULT_COLUMN_ALIAS_LENGTH - ORIGINAL_COLUMN_NAME_LENGTH - 1; | ||
|
||
String originalColumnNameTruncated = fixedLength(ORIGINAL_COLUMN_NAME_LENGTH) | ||
.split(column.getColumnName()) | ||
.iterator() | ||
.next(); | ||
String formatString = "%s_%0" + sequentialNumberLength + "d"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is formatString used? |
||
String columnName = originalColumnNameTruncated + "_" + padStart(Integer.toString(nextSyntheticColumnId), sequentialNumberLength, '0'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we pad the next synthetic id part, it leads to identifiers with long numbers in them. Is this required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also what happens when the |
||
return JdbcColumnHandle.builderFrom(column) | ||
.setColumnName(columnName) | ||
.build(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
/* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package io.trino.plugin.jdbc; | ||
|
||
import com.google.inject.AbstractModule; | ||
import com.google.inject.Singleton; | ||
|
||
public class ColumnWithAliasFormatterModule | ||
extends AbstractModule | ||
{ | ||
@Override | ||
public void configure() | ||
{ | ||
bind(ColumnWithAliasFormatter.class).in(Singleton.class); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,63 @@ | ||||||||||||||||||
/* | ||||||||||||||||||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||
* you may not use this file except in compliance with the License. | ||||||||||||||||||
* You may obtain a copy of the License at | ||||||||||||||||||
* | ||||||||||||||||||
* http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||
* | ||||||||||||||||||
* Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||
* distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||
* See the License for the specific language governing permissions and | ||||||||||||||||||
* limitations under the License. | ||||||||||||||||||
*/ | ||||||||||||||||||
package io.trino.plugin.jdbc; | ||||||||||||||||||
|
||||||||||||||||||
import com.google.inject.Inject; | ||||||||||||||||||
import io.trino.testing.TestingConnectorSession; | ||||||||||||||||||
import org.testng.annotations.Guice; | ||||||||||||||||||
import org.testng.annotations.Test; | ||||||||||||||||||
|
||||||||||||||||||
import static io.trino.plugin.jdbc.TestingJdbcTypeHandle.JDBC_VARCHAR; | ||||||||||||||||||
import static io.trino.spi.type.VarcharType.VARCHAR; | ||||||||||||||||||
import static org.assertj.core.api.Assertions.assertThat; | ||||||||||||||||||
|
||||||||||||||||||
@Guice(modules = ColumnWithAliasFormatterModule.class) | ||||||||||||||||||
public class TestColumnWithAliasFormatter | ||||||||||||||||||
{ | ||||||||||||||||||
@Inject | ||||||||||||||||||
private ColumnWithAliasFormatter actor; | ||||||||||||||||||
Comment on lines
+25
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
equivalent. |
||||||||||||||||||
|
||||||||||||||||||
private final TestingConnectorSession session = TestingConnectorSession.builder().build(); | ||||||||||||||||||
|
||||||||||||||||||
@Test | ||||||||||||||||||
public void testTooLongName() | ||||||||||||||||||
{ | ||||||||||||||||||
JdbcColumnHandle column = getDefaultColumnHandleBuilder() | ||||||||||||||||||
.setColumnName("column_with_over_twenty_characters") | ||||||||||||||||||
.build(); | ||||||||||||||||||
|
||||||||||||||||||
JdbcColumnHandle result = actor.format(session, column, 100); | ||||||||||||||||||
|
||||||||||||||||||
assertThat(result.getColumnName()).isEqualTo("column_with_over_twenty__00100"); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
@Test | ||||||||||||||||||
public void testTooShortName() | ||||||||||||||||||
{ | ||||||||||||||||||
JdbcColumnHandle column = getDefaultColumnHandleBuilder() | ||||||||||||||||||
.setColumnName("column_0") | ||||||||||||||||||
.build(); | ||||||||||||||||||
|
||||||||||||||||||
JdbcColumnHandle result = actor.format(session, column, 999); | ||||||||||||||||||
|
||||||||||||||||||
assertThat(result.getColumnName()).isEqualTo("column_0_00999"); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
private static JdbcColumnHandle.Builder getDefaultColumnHandleBuilder() | ||||||||||||||||||
{ | ||||||||||||||||||
return JdbcColumnHandle.builder() | ||||||||||||||||||
.setJdbcTypeHandle(JDBC_VARCHAR) | ||||||||||||||||||
.setColumnType(VARCHAR); | ||||||||||||||||||
} | ||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code lives in
trino-base-jdbc
but the values are specific to Oracle.Also consider adding a explanatory comment about why
ORIGINAL_COLUMN_NAME_LENGTH = DEFAULT_COLUMN_ALIAS_LENGTH - 6
. Where does the magic value 6 come from?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to ignore for now since as follow-up we'll make this general for all connectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still applies.