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.

Sign up for GitHub

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

Jump to bottom

Fix failure when ACID table column name collides with ACID internal names #12401

Merged
merged 1 commit into from
May 18, 2022
Merged

Fix failure when ACID table column name collides with ACID internal names #12401

merged 1 commit into from
May 18, 2022

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented May 16, 2022

Description

Fix failure when building column metadata in Hive

Documentation

(x) No documentation is needed.

Release notes

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

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 16, 2022
@ebyhr
Copy link
Member Author

ebyhr commented May 16, 2022

CI hit #12405

@ebyhr ebyhr requested review from findepi and electrum May 16, 2022 07:26
findepi
findepi reviewed May 16, 2022
View reviewed changes
plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
@@ -3411,29 +3405,12 @@ private static Function<HiveColumnHandle, ColumnMetadata> columnMetadataGetter(T
}
}

// add hidden columns
builder.put(PATH_COLUMN_NAME, Optional.empty());
Copy link
Member

Choose a reason for hiding this comment

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

why removed?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Show resolved Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java
"test_hidden_column_name_conflict",
format("(\"%s\" int, _bucket int, _partition int) WITH (partitioned_by = ARRAY['_partition'], bucketed_by = ARRAY['_bucket'], bucket_count = 10)", columnName))) {
assertThatThrownBy(() -> query("SELECT * FROM " + table.getName()))
.hasMessageContaining("Multiple entries with same key: " + columnName);
Copy link
Member

Choose a reason for hiding this comment

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

We would probably want disallow creation of such tables

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #12449

@findepi
Copy link
Member

findepi commented May 16, 2022

What problem(s) is this fixing?

@ebyhr
Copy link
Member Author

ebyhr commented May 16, 2022

@findepi The below buildOrThrow() in HiveMetadata.columnMetadataGetter() threw an exception when the table contains column names in AcidSchema.ACID_COLUMN_NAMES (e.g. operation) and it's the ACID table.

        Map<String, Optional<String>> columnComment = builder.buildOrThrow();

@findepi
Copy link
Member

findepi commented May 16, 2022

@findepi The below buildOrThrow() in HiveMetadata.columnMetadataGetter() threw an exception when the table contains column names in AcidSchema.ACID_COLUMN_NAMES (e.g. operation) and it's the ACID table.

        Map<String, Optional<String>> columnComment = builder.buildOrThrow();

Thanks for explanation. This makes this change clear to me now.

Please add mention of that in the commit message

@ebyhr ebyhr force-pushed the ebi/hive-hidden-column-comment branch from 75c352f to a3646b4 Compare May 17, 2022 00:01
@ebyhr
Copy link
Member Author

ebyhr commented May 17, 2022

@findepi Updated the commit message.

@findepi findepi changed the title Fix failure when building column metadata in Hive Fix failure when ACID table column name collides with ACID internal names May 17, 2022
@findepi findepi force-pushed the ebi/hive-hidden-column-comment branch from a3646b4 to 4eff038 Compare May 17, 2022 11:27
@findepi
Copy link
Member

findepi commented May 17, 2022

pushed; updated the commit title only.

@ebyhr
Fix failure when ACID table column name collides with ACID internal n…
cc9bede 
…ames

Previously, HiveMetadata.columnMetadataGetter threw
an exception due to duplicated keys when the table
contains column names of ACID format columns
(e.g. operation) and it's ACID table.
@ebyhr ebyhr force-pushed the ebi/hive-hidden-column-comment branch from 4eff038 to cc9bede Compare May 17, 2022 23:53
@ebyhr
Copy link
Member Author

ebyhr commented May 17, 2022

Thanks, removed my old commit title.
Screen Shot 2022-05-18 at 8 52 42

@ebyhr ebyhr merged commit 50582d2 into trinodb:master May 18, 2022
@ebyhr ebyhr deleted the ebi/hive-hidden-column-comment branch May 18, 2022 02:36
@ebyhr ebyhr mentioned this pull request May 18, 2022
Closed
@github-actions github-actions bot added this to the 382 milestone May 18, 2022
@mosabua mosabua mentioned this pull request May 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed
Milestone
382
Development

Successfully merging this pull request may close these issues.
2 participants
@ebyhr @findepi