Skip to content

feat(catalog/sql): Add views related ops #414

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
Merged
zeroshade merged 16 commits into from
Jun 20, 2025
Merged

feat(catalog/sql): Add views related ops #414

zeroshade merged 16 commits into from
Jun 20, 2025

Conversation

@dttung2905
Copy link
Contributor

@dttung2905 dttung2905 commented May 1, 2025

No description provided.

@dttung2905 dttung2905 marked this pull request as ready for review May 6, 2025 19:09
dttung2905
dttung2905 commented May 6, 2025
View reviewed changes
zeroshade
zeroshade reviewed May 8, 2025
View reviewed changes
zeroshade
zeroshade reviewed May 8, 2025
View reviewed changes
@Shreyas220
Copy link
Contributor

Shreyas220 commented May 13, 2025

Hey @dttung2905, just wanted to check if you're still actively working on this PR. If not, I'd be happy to pick it up and continue. Let me know your thoughts!

@dttung2905
Copy link
Contributor Author

dttung2905 commented May 13, 2025

Hey @dttung2905, just wanted to check if you're still actively working on this PR. If not, I'd be happy to pick it up and continue. Let me know your thoughts!

Hello 👋, yes, i was still working on it. Its taking longer than usual sorry. Will update the code some time today for review

@dttung2905 dttung2905 force-pushed the add-views-related-ops-sql branch from 80ce8a4 to 2b6e025 Compare May 18, 2025 17:10
zeroshade
zeroshade reviewed May 19, 2025
View reviewed changes
lliangyu-lin
lliangyu-lin reviewed May 20, 2025
View reviewed changes
lliangyu-lin
lliangyu-lin reviewed May 20, 2025
View reviewed changes
zeroshade
zeroshade reviewed May 23, 2025
View reviewed changes
zeroshade
zeroshade requested changes May 23, 2025
View reviewed changes
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

just a few comments but overall looks good to me so far.

Is there any way we can set up an integration test to compare against other implementations?

@dttung2905
Copy link
Contributor Author

dttung2905 commented May 26, 2025

just a few comments but overall looks good to me so far.

Is there any way we can set up an integration test to compare against other implementations?

@zeroshade Thanks for the review. I have made some changes for the linting which should make the CI passes. Regarding the integration test, I think its a good idea. I'm working on something similar to rest_integration_test.go but for SQL catalog. Since this PR is getting substantially big to review, can I include the integration in the next PR ? wdyt ?

@zeroshade
Copy link
Member

zeroshade commented May 26, 2025

A follow up PR for the integration test is fine. If you're not gonna start immediately after this gets merged, then let's file an issue to track it so we don't forget. But if you'll be following up quickly with the test, no need for an issue. 😄

@dttung2905 dttung2905 mentioned this pull request May 27, 2025
Closed
@dttung2905
Copy link
Contributor Author

dttung2905 commented May 27, 2025

@zeroshade It seems like the go-integration test is failing which other PR also encounters. I don't think it is related to any changes related to this PR. Do you have any idea how to fix it? I will try to rebase from main to see if the CI passes

@dttung2905 dttung2905 force-pushed the add-views-related-ops-sql branch from 62aa3b3 to 43a8540 Compare May 27, 2025 22:45
@zeroshade
Copy link
Member

zeroshade commented May 28, 2025

@dttung2905 this is super weird. I can't replicate the failure locally. It looks like it's getting some kind of error trying to write to the minio instance. @Fokko @kevinjqliu any ideas what could be going wrong here? Looks like it's failing inside of pyiceberg's catalog.create_table call saying the specified bucket doesn't exist in the minio instance.

@zeroshade
Copy link
Member

zeroshade commented May 28, 2025

@dttung2905 figured out the issue, #444 fixes it so you can rebase once that gets merged.

@dttung2905
Copy link
Contributor Author

dttung2905 commented May 28, 2025

@dttung2905 figured out the issue, #444 fixes it so you can rebase once that gets merged.

Sure. Let me rebase it again in a few hours. Thanks alot for the quick fix

dttung2905 added 10 commits May 28, 2025 21:59
@dttung2905
Add view related operation for SQL
2c8fa0b 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add view related operation for SQL
70cdbe2 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add test cases
3ab0715 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add README file
032a9a0 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Merge to a single sqlIcebergTable struct
bc82cd0 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Remove unused struct
cc0c6dc 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add actual metadata path
b7e5505 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Minor code review and adding Operation in the ViewVersion struct
93ed7a6 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix alignment that was not shown clearly on ide
fb44ff4 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Add TableType and ViewType const and fix linter issue
fa60382 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix linting
935cad1 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905 dttung2905 force-pushed the add-views-related-ops-sql branch from 43a8540 to 935cad1 Compare May 28, 2025 21:00
@dttung2905 dttung2905 requested a review from zeroshade May 29, 2025 22:55
@dttung2905 dttung2905 mentioned this pull request Jun 3, 2025
Merged
zeroshade
zeroshade reviewed Jun 5, 2025
View reviewed changes
zeroshade
zeroshade reviewed Jun 5, 2025
View reviewed changes
zeroshade
zeroshade reviewed Jun 5, 2025
View reviewed changes
@dttung2905
Fixes from code review
91cdcc1 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905 dttung2905 requested a review from zeroshade June 10, 2025 20:32
@dttung2905
Fix golangci error output
93b4e3d 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Fix ci lint error
191f9aa 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
@dttung2905
Merge branch 'apache:main' into add-views-related-ops-sql
16c85ce
@dttung2905
Fix ci lint error
8a0d06d 
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
zeroshade
zeroshade approved these changes Jun 20, 2025
View reviewed changes
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@zeroshade zeroshade merged commit 60abc85 into apache:main Jun 20, 2025
10 checks passed
zeroshade pushed a commit that referenced this pull request Jun 25, 2025
@dttung2905
feat(catalog/sql): Add integration test for sql catalog (#450)
4d06fea 
Hi team,
This PR fixes #442

Will need the PR #414 to be
merged first before I can uncomment one of the integration test case

---------

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeroshade zeroshade zeroshade approved these changes

+1 more reviewer

@lliangyu-lin lliangyu-lin lliangyu-lin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dttung2905 @Shreyas220 @zeroshade @lliangyu-lin