Skip to content

feat [python]: Add getter/setter property for writer_bucket_no_key_assigner config#397

Merged
luoyuxia merged 4 commits into
apache:mainfrom
charlesdong1991:add-getter-no-key-assigner
Mar 5, 2026
Merged

feat [python]: Add getter/setter property for writer_bucket_no_key_assigner config#397
luoyuxia merged 4 commits into
apache:mainfrom
charlesdong1991:add-getter-no-key-assigner

Conversation

@charlesdong1991

Copy link
Copy Markdown
Contributor

After my previous PR for updating writer_bucket_no_key_assigner config, I realize unlike other configs, this config does not have getter/setter property.

So this PR adds it up. No tests should be needed.

@charlesdong1991

Copy link
Copy Markdown
Contributor Author

@luoyuxia @fresh-borzoni @leekeiabstraction PTAL. Thanks!

@fresh-borzoni

Copy link
Copy Markdown
Member

@charlesdong1991 TY for the PR

Small comment:
The same match value { "round_robin" | "sticky" } now exists in 3 places (dict constructor, this setter, Cpp new_connection). NoKeyAssigner already derives ValueEnum but is missing FromStr.

strum is already a dep and DataLakeFormat already uses the exact pattern needed, we can do like this:

  #[derive(..., EnumString, Display)]                                                                                                                                                                                                                
  #[strum(ascii_case_insensitive)]                      
  pub enum NoKeyAssigner {
      #[strum(serialize = "sticky")]
      Sticky,
      #[strum(serialize = "round_robin")]
      RoundRobin,
  }

Then all 3 sites become value.parse().map_err(...) — same as the numeric fields. Also makes parsing case-insensitive for free.

@charlesdong1991

Copy link
Copy Markdown
Contributor Author

Good call, we can indeed de-duplicate and clean codebase! @fresh-borzoni 👍

@charlesdong1991

Copy link
Copy Markdown
Contributor Author

I did the changes, please let me know if any further changes! Thanks again! @fresh-borzoni

@fresh-borzoni fresh-borzoni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@charlesdong1991 Ty, LGTM

@leekeiabstraction leekeiabstraction left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ty for the pr!

@luoyuxia luoyuxia merged commit dd10579 into apache:main Mar 5, 2026
8 checks passed
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.

4 participants