Skip to content

Implement mode function - #12385

Closed
s5dsn-eqee wants to merge 1 commit into
apache:mainfrom
s5dsn-eqee:agg-mode-impl
Closed

s5dsn-eqee wants to merge 1 commit into
apache:mainfrom
s5dsn-eqee:agg-mode-impl

Conversation

@s5dsn-eqee

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #12248

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Sep 8, 2024
@alamb

alamb commented Sep 16, 2024

Copy link
Copy Markdown
Contributor

RElated comment: #12254 (comment)

@s5dsn-eqee

Copy link
Copy Markdown
Contributor Author

Hi @alamb,

Could you help me choose the right approach?

I’ve started working on this issue. We chose "mode" as the first function for datafusion-functions-extra, but I’ve run into a challenge. My implementation involves patching ArrowBytesMap with new methods.

Should I first patch ArrowBytesMap and then wait for the new release of the DataFusion core, or is there a more efficient way to proceed?

@alamb

alamb commented Sep 20, 2024

Copy link
Copy Markdown
Contributor

Should I first patch ArrowBytesMap and then wait for the new release of the DataFusion core, or is there a more efficient way to proceed?

I would recommend:

  1. Make a PR to update the new method
  2. In the code of datafusion-functions-extra make a copy of ArrowBytesMap with the new methods you need with a link back to the PR that adds the APIs

That way you'll be unblocked but still have a path to avoid a second copy of the bytes map

@s5dsn-eqee

Copy link
Copy Markdown
Contributor Author

@alamb thank you!

@s5dsn-eqee

Copy link
Copy Markdown
Contributor Author

Implemented: mode.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support mode in Aggregation function

2 participants