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

fix: expose the fluent API fn for approx_distinct instead of the module #11644

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

Michael-J-Ward
Copy link
Contributor

Fixes: #11643

Which issue does this PR close?

Closes #11643 .

Rationale for this change

expr_fn module should aggregate and expose the fluent APIs, not the modules which are exposed separately.

What changes are included in this PR?

Update the re-export in expr_fn to expose the fluent API.

Are these changes tested?

No additional tests.

Are there any user-facing changes?

Anyone that currently references functions_aggregate::expr_fn::approx_distinct::aprox_distinct will need to change that reference.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @Michael-J-Ward

@jayzhan211
Copy link
Contributor

approx_distinct is missing in roundtrip_expr_api, that is the reason we didn't catch the bug here, could you also add approx_distinct to it, thanks!

@jayzhan211
Copy link
Contributor

Thanks @Michael-J-Ward @alamb

@jayzhan211 jayzhan211 merged commit f12b3db into apache:main Jul 25, 2024
24 checks passed
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Aug 10, 2024
Michael-J-Ward added a commit to Michael-J-Ward/datafusion-python that referenced this pull request Aug 20, 2024
andygrove pushed a commit to apache/datafusion-python that referenced this pull request Aug 23, 2024
* update datafusion deps to point to githuc.com/apache/datafusion

Datafusion 41 is not yet released on crates.io.

* update TableProvider::scan

Ref: apache/datafusion#11516

* use SessionStateBuilder

The old constructor is deprecated.

Ref: apache/datafusion#11403

* update AggregateFunction

Upstream Changes:
- The field name was switched from `func_name` to func.
- AggregateFunctionDefinition was removed

Ref: apache/datafusion#11803

* update imports in catalog

Catlog API was extracted to a separate crate.

Ref: apache/datafusion#11516

* use appropriate path for approx_distinct

Ref: apache/datafusion#11644

* migrate AggregateExt to ExprFunctionExt

Also removed `sqlparser` dependency since it's re-exported upstream.

Ref: apache/datafusion#11550

* update regr_count tests for new return type

Ref: apache/datafusion#11731

* migrate from function-array to functions-nested

The package was renamed upstream.

Ref: apache/datafusion#11602

* cargo fmt

* lock datafusion deps to 41

* remove todo from cargo.toml

All the datafusion dependencies are re-exported, but I still need to figure out *why*.
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.

functions_aggregate::expr_fn::approx_distinct should expose the function, not the module
3 participants