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

Move load and upload out of binder. #509

Merged
merged 22 commits into from
Dec 1, 2022

Conversation

xzdandy
Copy link
Collaborator

@xzdandy xzdandy commented Nov 25, 2022

Due to the explain command, the catalog operations required by load and upload can not be completed in the binder. Move them to executor instead.

@xzdandy xzdandy marked this pull request as ready for review November 29, 2022 01:11
@xzdandy xzdandy linked an issue Nov 30, 2022 that may be closed by this pull request
@xzdandy xzdandy requested a review from gaurav274 November 30, 2022 19:40
@xzdandy xzdandy requested a review from jiashenC December 1, 2022 19:16
Copy link
Member

@gaurav274 gaurav274 left a comment

Choose a reason for hiding this comment

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

LGTM

ColumnDefinition(
"name", ColumnType.TEXT, None, [], ColConstraintInfo(unique=True)
),
ColumnDefinition("id", ColumnType.INTEGER, None, []),
Copy link
Member

Choose a reason for hiding this comment

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

Do we still want to use a explicit id column or change to _row_id column?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jiashenC The function is moved out from the binder for clarification (since it is not used in binder anymore). No modifications are made. Regarding the id vs _row_id, I do not have preference, either works.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, just get your opinion if we can get rid of this redundant column. Others LGTM.

Copy link
Collaborator Author

@xzdandy xzdandy Dec 1, 2022

Choose a reason for hiding this comment

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

Just come to my mind, under the current implementation. id and _row_id are different when a dataset has more than 1 video. id restarts from 0/1 for every video. _row_id probably should not.

@xzdandy xzdandy merged commit 376820f into georgia-tech-db:master Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code inconsistency
3 participants