-
Notifications
You must be signed in to change notification settings - Fork 0
Export CSV config revamp to support table exports #199
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
Open
davidwzhao
wants to merge
34
commits into
main
Choose a base branch
from
dz-csv-export-table-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
29b01f9
Update ExportCSVConfig to support generic CSV sources
davidwzhao ae4cac5
Update generated proto files
davidwzhao 4e777d1
Update grammar
davidwzhao 5ab4715
Try using Any types
davidwzhao da6d2a9
Add partition size to csv config
davidwzhao c44d462
Use separate options for legacy vs new csv export config
davidwzhao 7d4b9a0
Update generated proto and parser
davidwzhao 33073d1
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao dbf57a7
Update partition_size to partition_size_mb
davidwzhao 1f53c82
Update grammar to distinguish between new and legacy ExportCSVConfig …
davidwzhao 15b982d
Update pretty printer generator to support field unwrapping under con…
davidwzhao c2a838b
Update generated sdks
davidwzhao 7d4bc26
Format
davidwzhao 1e297f6
Add tests for export_csv_config_v2
davidwzhao 18f2f69
Fix type error
davidwzhao 0137a34
Update generated parser
davidwzhao 7b125a9
Go back to having separate grammar rule for export_csv_columns
davidwzhao 468d657
Update generated parsers and printers
davidwzhao 028f7f9
Update Go pretty printer
davidwzhao 6993438
Update pretty snapshots
davidwzhao 150f48b
Add csv_export attr to test
davidwzhao 7600ca0
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao bf6c9a2
No need for ignoring validator
davidwzhao 8528f23
Update snaps
davidwzhao b5c3da9
Update go snaps
davidwzhao 7fa4441
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao aa9ca13
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao 9de602e
Update grammar
davidwzhao 7aaddda
Fix grammar
davidwzhao 5cf9906
Add debug snaps for export tests
davidwzhao 7aa23eb
Add return type annotation
davidwzhao 87e7bd5
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao 8f71193
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao 842f291
Merge remote-tracking branch 'origin/main' into dz-csv-export-table-2
davidwzhao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,9 @@ message ExportCSVConfig { | |||||||
| string path = 1; | ||||||||
| repeated ExportCSVColumn data_columns = 2; | ||||||||
|
|
||||||||
| ExportCSVSource csv_source = 10; | ||||||||
| CSVConfig csv_config = 11; | ||||||||
|
|
||||||||
| optional int64 partition_size = 3; | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| optional string compression = 4; | ||||||||
| optional bool syntax_header_row = 5; | ||||||||
|
|
@@ -95,6 +98,17 @@ message ExportCSVColumn { | |||||||
| RelationId column_data = 2; | ||||||||
| } | ||||||||
|
|
||||||||
| message ExportCSVColumns { | ||||||||
| repeated ExportCSVColumn columns = 1; | ||||||||
| } | ||||||||
|
|
||||||||
| message ExportCSVSource { | ||||||||
| oneof csv_source { | ||||||||
| ExportCSVColumns gnf_columns = 1; | ||||||||
| RelationId table_def = 2; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // | ||||||||
| // Read operations | ||||||||
| // | ||||||||
|
|
||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this about? 🤔 Is this from not declaring "optional" that we were discussing with @nystrom ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the grammar i directly construct the
ExportCSVSourcefrom columns, rather than having an intermediate - see belowThat means that there's no grammar rule that actually produces an
ExportCSVColumnsobject on its own. And no, i think this is independent of theoptionalthing