Skip to content

fix(trino): the error when fetching Hudi tables Schema - #26131

Closed
LittleWat wants to merge 2 commits into
apache:masterfrom
LittleWat:fix-fetching-trino-hudi-table-schema
Closed

fix(trino): the error when fetching Hudi tables Schema#26131
LittleWat wants to merge 2 commits into
apache:masterfrom
LittleWat:fix-fetching-trino-hudi-table-schema

Conversation

@LittleWat

@LittleWat LittleWat commented Nov 29, 2023

Copy link
Copy Markdown

SUMMARY

$partitions is not necessary so let me remove that.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@LittleWat
fix(trino): the error when fetching Hudi tables Schema
6352fcf 
- This PR attempts to close apache#21945

`$partitions` is not necessary so let me remove that.
@LittleWat
LittleWat force-pushed the fix-fetching-trino-hudi-table-schema branch from 4468a81 to 6352fcf Compare November 29, 2023 13:57
@john-bodley john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Nov 29, 2023
john-bodley
john-bodley reviewed Nov 29, 2023
View reviewed changes
Comment thread superset/db_engine_specs/presto.py
partition_select_clause = f"SHOW PARTITIONS FROM {full_table_name}"
else:
system_table_name = f'"{table_name}$partitions"'
system_table_name = f'"{table_name}$partitions"' if not is_trino else table_name

@john-bodley john-bodley Nov 29, 2023 •
edited
Loading

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.

@LittleWat note that Presto does support the $partitions table suffix per the release notes. Are you saying that Hudi (which I'm not well versed with) doesn't?

@LittleWat LittleWat Nov 30, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thank you for checking! Presto+Hudi works but Trino+Hudi does not work. I'm wondering if Trino supports the $partitions table suffix 🤔

@LittleWat LittleWat Nov 30, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

even after I made this change, I got the error so I made a draft PR in trino-python-client repo:
trinodb/trino-python-client#426

Comment thread superset/db_engine_specs/presto.py
limit: int = 0,
order_by: list[tuple[str, bool]] | None = None,
filters: dict[Any, Any] | None = None,
is_trino: bool = False,

@john-bodley john-bodley Nov 29, 2023

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.

If this is the route we want to take, ideally this should be not Trino specific, i.e., maybe something like use_partition_suffix.

@john-bodley
john-bodley self-requested a review November 29, 2023 18:01
@john-bodley

john-bodley commented Nov 29, 2023

Copy link
Copy Markdown
Member

Thanks @LittleWat for the PR. I've kicked off CI and added a few comments.

@codecov

codecov Bot commented Nov 29, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (b9aaf0f) 58.93% compared to head (1abbfe8) 67.04%.
Report is 3 commits behind head on master.

Files Patch % Lines
superset/db_engine_specs/trino.py 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #26131      +/-   ##
==========================================
+ Coverage   58.93%   67.04%   +8.11%     
==========================================
  Files        1941     1941              
  Lines       75873    75876       +3     
  Branches     8447     8447              
==========================================
+ Hits        44714    50872    +6158     
+ Misses      28984    22829    -6155     
  Partials     2175     2175
Flag Coverage Δ
hive 53.68% <0.00%> (-0.01%) ⬇️
mysql 78.15% <25.00%> (?)
postgres 78.25% <25.00%> (?)
presto 53.63% <0.00%> (-0.01%) ⬇️
python 78.66% <25.00%> (+16.87%) ⬆️
sqlite 76.90% <25.00%> (?)
unit ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Nov 30, 2023
@LittleWat

LittleWat commented Dec 25, 2023

Copy link
Copy Markdown
Author

Let me close this PR because this was fixed by updating the trino-python-client (trinodb/trino-python-client#426)

pip install git+https://github.com/trinodb/trino-python-client.git@9df2cf2756d9bb189de719a80623064acfe65dea

Fixed the error.

@LittleWat LittleWat closed this Dec 25, 2023
@LittleWat
LittleWat deleted the fix-fetching-trino-hudi-table-schema branch December 25, 2023 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@john-bodley john-bodley Awaiting requested review from john-bodley

Assignees

No one assigned

Labels

size/XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error when fetching Hudi tables Schema

2 participants

@LittleWat @john-bodley