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.
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
parse table metadata.configuration as
TableProperties
#453parse table metadata.configuration as
TableProperties
#453Changes from 3 commits
ba8309e
9d4b599
1b7b193
02d50ee
e4676d6
9f8afa4
ed2c10a
42e6028
f1b9a16
82370b4
00b9d8e
f748f87
af08092
4587794
1e7d286
bd9ac7a
fa48054
ff78623
b667a15
b3cdc61
a891b52
d8a2933
d1ce73d
d8af98c
6d1b466
f18b885
437b8db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
only meaningful change is adding
pub mod table_properties
, other changes are shifting to colocate module declarationsThere 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.
We don't normally use
get_
for field accessors?(rust can handle structs with both
foo: Foo
andfn foo(&self) -> &Foo
-- even if both are public)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.
I'm curious to hear everyone's thoughts on this.. I think we need two things here (and the API above was attempting to answer the second part)
TableProperties
struct) which lets users (or engines) examine the table properties that are setthis probably isn't the PR for tackling (2) - but would love to hear some thoughts so we could get started on a follow-up. For now I'll just do
column_mapping_mode()
to unblock this and we can iterate?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.
cc @nicklan @scovich
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.
I think it makes a lot of sense. These are checks that are going to be made for every table feature, so it helps to have all of it in one place.
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.
Yeah. Likely we don't need a method for each one and should rather check a struct of some sort. The API design is a bit subtle though, as some features can be set to multiple modes (i.e. column mapping) and others can just be on/off.
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.
just gonna take it as a follow up #508
and as @nicklan suggested: probably just wait a little longer until we have more use cases and then can inform how we want to build something that unifies protocol/table properties
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.
aside: Do these classes still need serde support?
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.
unfortunately yea, since it is still a field in
GlobalScanState
(which derivesSerialize
/Deserialize
)