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

Modify layout of grouping files #4525

Merged
merged 9 commits into from
Oct 31, 2023

Conversation

malhotrashivam
Copy link
Contributor

@malhotrashivam malhotrashivam commented Sep 19, 2023

Closes #4283

Proposal document for new layout.

@malhotrashivam malhotrashivam added bug Something isn't working parquet Related to the Parquet integration NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Sep 19, 2023
@malhotrashivam malhotrashivam added this to the September 2023 milestone Sep 19, 2023
@malhotrashivam malhotrashivam self-assigned this Sep 19, 2023
@malhotrashivam malhotrashivam changed the title Modify layout of grouping files (WIP) Modify layout of grouping files Sep 21, 2023
@malhotrashivam malhotrashivam changed the title Modify layout of grouping files Modify layout of grouping files (WIP) Sep 21, 2023
@malhotrashivam malhotrashivam changed the title Modify layout of grouping files (WIP) (WIP) Modify layout of grouping files Oct 4, 2023
@malhotrashivam malhotrashivam changed the title (WIP) Modify layout of grouping files Modify layout of grouping files Oct 11, 2023
Copy link
Contributor

@lbooker42 lbooker42 left a comment

Choose a reason for hiding this comment

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

Only one actual comment, others are cosmetic.

lbooker42
lbooker42 previously approved these changes Oct 18, 2023
Copy link
Contributor

@lbooker42 lbooker42 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. A few minor comments, and a suggestion for further fallback.

final String prefix = minusParquetSuffix(path);
return columnName -> prefix + "_" + columnName + "_grouping.parquet";
/**
* Generates a grouping file path relative to the table destination file path.
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were re-branding "grouping" to "indexing" in this PR for new/changed code, to avoid creating a subsequent task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lbooker42 mentioned that he will be doing renaming in his changes, so I didn't do it here.

Copy link
Member

Choose a reason for hiding this comment

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

My complaint was more that we're adding more work with some of the new code, and creating a "moving target" for him.

Copy link
Contributor

@lbooker42 lbooker42 left a comment

Choose a reason for hiding this comment

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

The renaming of group to index looks good. Considering the code base still supports grouping (and not multi-column indexes) I think this is as far as we should go in this PR.

@malhotrashivam malhotrashivam force-pushed the sm-grouping-dir branch 3 times, most recently from e50c4b4 to ec762d3 Compare October 31, 2023 16:23
@malhotrashivam malhotrashivam merged commit bc26812 into deephaven:main Oct 31, 2023
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. parquet Related to the Parquet integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grouping files for parquet should not be written in the same directory as the main file
3 participants