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.

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

Remove Pinot Column Name Property #14086

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Conversation

elonazoulay
Copy link
Member

@elonazoulay elonazoulay commented Sep 9, 2022

Description

Add the column name property to Pinot column properties.
Fixes #14071

Non-technical explanation

The pinot column name property was missing so SHOW CREATE TABLE queries would fail.

Release notes

( ) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Pinot
* Fix failure when executing `SHOW CREATE TABLE` statement. ({issue}`14071`)

@findepi
Copy link
Member

findepi commented Oct 3, 2022

What's the "column name property" and how is it different from the column name?

cc @martint

@elonazoulay
Copy link
Member Author

@findepi - The column name property is the pinot column name which may contain non lowercase characters.

@elonazoulay
Copy link
Member Author

elonazoulay commented Oct 8, 2022

Looks like the hidden properties are included show create table.
Should hidden column properties be filtered out in ShowQueriesRewrite (below)?

Collection<PropertyMetadata<?>> allColumnProperties = columnPropertyManager.getAllProperties(tableHandle.getCatalogHandle());

@ebyhr @findepi

@findepi
Copy link
Member

findepi commented Oct 10, 2022

@findepi - The column name property is the pinot column name which may contain non lowercase characters.

We don't use "column property" in any other connector. Why did we consider Pinot to be special and require different approach?

@elonazoulay
Copy link
Member Author

I don't want it to be special in that way:) iirc this was from the first version. Instead should I just keep a mapping?
Instead of being able to create a PinotColumnHandle directly from column metadata, should it look up the pinot "raw" column name? Assuming that's what you mean, lmk.

@findepi
Copy link
Member

findepi commented Oct 12, 2022

If agree that "column property" is the way to go, that's what we should be doing in other connectors, like PostgreSQL, MySQL, SQL Server, etc.
if we agree that "column property" way is not superior, Pinot connector shouldn't be doing that (unless there is some Pinot-specific reason to)

@elonazoulay
Copy link
Member Author

I think it's better to remove it: since no other connectors use it and there is a way to do this without the column property then it makes more sense to remove it. Refactoring this pr. wdyt? (I'll save this branch just in case).

@elonazoulay elonazoulay changed the title Add column name property to Pinot Remove Pinot column name property Oct 15, 2022
@elonazoulay elonazoulay changed the title Remove Pinot column name property Remove Pinot Column Name Property Oct 15, 2022
@elonazoulay
Copy link
Member Author

elonazoulay commented Oct 15, 2022

Whenever you have a chance, I updated the pr to remove the property cc @findepi @ebyhr

@ebyhr ebyhr merged commit 5f28e8d into trinodb:master Oct 25, 2022
@ebyhr
Copy link
Member

ebyhr commented Oct 25, 2022

Merged, thanks!

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

Pinot connector throws No PropertyMetadata for property: pinotColumnName in SHOW CREATE TABLE
3 participants