Skip to content

Fix inserting into table with identifiers with quotes#13237

Merged
ebyhr merged 2 commits intotrinodb:masterfrom
homar:homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails
Jul 23, 2022
Merged

Fix inserting into table with identifiers with quotes#13237
ebyhr merged 2 commits intotrinodb:masterfrom
homar:homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented Jul 19, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

a connector

How would you describe this change to a non-technical end user or system administrator?

It fixes inserts for tables with columns with quotes in their names

Related issues, pull requests, and links

Fixes: #13074

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

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

# Iceberg
* Fix failure when inserting into a Parquet table with columns with quotes in names. ({issue}`13074`)

@cla-bot cla-bot bot added the cla-signed label Jul 19, 2022
@homar homar requested review from alexjo2144 and findepi July 19, 2022 17:46
@alexjo2144
Copy link
Copy Markdown
Member

Maven checks are complaining, but otherwise looks good to me

@alexjo2144
Copy link
Copy Markdown
Member

Can you add a test to BaseIcebergIntegrationTest as well?

@homar
Copy link
Copy Markdown
Member Author

homar commented Jul 19, 2022

Maven checks are complaining, but otherwise looks good to me

yeah I forgot about header :D

@homar
Copy link
Copy Markdown
Member Author

homar commented Jul 19, 2022

Can you add a test to BaseIcebergIntegrationTest as well?

I thought about that but decided it is more optimal to just unit test this. But sure I can add it.

@homar homar force-pushed the homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails branch from 2e42758 to 5961778 Compare July 19, 2022 20:55
@homar
Copy link
Copy Markdown
Member Author

homar commented Jul 19, 2022

@alexjo2144 comments applied

@electrum
Copy link
Copy Markdown
Member

Shouldn't this have release notes?

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 assume the uppercase T is not intentional, and it distracts from the purpose of the test.

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.

Let's call the row x since "a row" sounds like English and blends in with the other identifiers.

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.

ImmutableMap.of() can be hard to read, since the key/value pairs blend together. We can write this as

assertThat(PrimitiveTypeMapBuilder.makeTypeMap(inputTypes, inputColumnNames).containsExactly(
        Map.entry(List.of("an_x20identifier_x20with_x20_x22quotes_x22_x20"), INTEGER),
        Map.entry(List.of("a", "another_x20identifier"), INTEGER));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I will do it but with ImmutableList instead of List as I think in general we use ImmutableList much more often

@homar
Copy link
Copy Markdown
Member Author

homar commented Jul 20, 2022

Shouldn't this have release notes?

Ok

@homar homar force-pushed the homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails branch 2 times, most recently from 45a87af to 1103bdd Compare July 20, 2022 10:58
@ebyhr ebyhr force-pushed the homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails branch from 1103bdd to 97a5944 Compare July 20, 2022 23:29
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jul 20, 2022

Just added DROP TABLE at the end of test and updated the commit title.

@homar homar force-pushed the homar/iceberg_insert_into_tables_with_identifiers_with_quotes_fails branch from 58b0abd to 97a5944 Compare July 22, 2022 10:26
@ebyhr ebyhr merged commit fd82ae5 into trinodb:master Jul 23, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Jul 23, 2022

Merged, thanks!

@ebyhr ebyhr mentioned this pull request Jul 23, 2022
@github-actions github-actions bot added this to the 392 milestone Jul 23, 2022
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.

Cannot insert into Iceberg Parquet table having column identifier with quotes

4 participants