[branch-51] Back port CatalogProviderList to DF 51 #18911
Closed
Conversation
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
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - part of apache#17558 ## Rationale for this change Let's update the version numbers! ## What changes are included in this PR? Update numbers and add changelog. I plan to merge this PR now and I will make a follow on PR (and forward port it to main) with final touchups ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
…ion (apache#18567) Note this targets the branch-51 release branch ## Which issue does this PR close? - part of apache#17558 - resolves apache#17801 in the 51 release branch ## Rationale for this change - We merged some clever rewrites for `coalesce` and `nvl2` to use `CASE` which are faster and more correct (👏 @chenkovsky @kosiew ) - However, these rewrites cause subtle schema mismatches in some cases planning (b/c the CASE simplification nullability logic can't determine the correct nullability in some cases - see apache#17801) - @pepijnve has some heroic efforts to fix the schema mismatch in apache#17813 (comment), but it is non trivial and I am worried about merging it so close to the 51 release and introducing new edge cases ## What changes are included in this PR? 1. Revert apache#17357 / e5dcc8c 3. Revert apache#17991 / ea83c26 2. Revert apache#18191 / 22c4214 2. Cherry-pick 6202254, a test that reproduces the schema mismatch issue (from apache#18536) 3. Cherry-pick 735cacf, a fix for the benchmarks that regressed due to the revert (from apache#17833) 4. Update datafusion-testing (see separate PR here) for extended tests (see apache/datafusion-testing#15) ## Are these changes tested? Yes I added a new test ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
## Which issue does this PR close? - part of apache#17558 ## Rationale for this change Per @xudong963 in apache#18567 (comment), we should update the changelog on `branch-51` to incorporate - apache#18567 ## What changes are included in this PR? Ran ```shell ./dev/release/generate-changelog.py 50.3.0 branch-51 51.0.0 > dev/changelog/51.0.0.md npx prettier@2.7.1 --write '{datafusion,datafusion-cli,datafusion-examples,dev,docs}/**/*.md' '!datafusion/CHANGELOG.md' README.md CONTRIBUTING.md ``` And checked in the results ## Are these changes tested? By CI ## Are there any user-facing changes? New changelog content
…F51 (apache#18618) This is the same as apache#18617 but targeted at the DF-51 release branch
…che#18629) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#18597 ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> A check is recently added to `invoke_with_args` that checks for the output type of the result with the expected output type from the UDF - apache#17515. Because the fast path misses adding the timezone, the assertion added in this PR fails. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Include timezone information in the fast path. ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes, added a unit test and SLT test ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> No <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
…che#18624) This is a duplicate of apache#18623 but targeting branch51 See that PR for details.
## Which issue does this PR close? - part of apache#17558 (comment) ## Rationale for this change Per @xudong963 apache#17558 (comment) we have backported stuff into the 51 branch, so we also need to update the changelog ## What changes are included in this PR? Ran ```shell ./dev/release/generate-changelog.py 50.3.0 branch-51 51.0.0 > dev/changelog/51.0.0.md npx prettier@2.7.1 --write '{datafusion,datafusion-cli,datafusion-examples,dev,docs}/**/*.md' '!datafusion/CHANGELOG.md' README.md CONTRIBUTING.md ``` And checked in the result ## Are these changes tested? By CI ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
## Which issue does this PR close? None ## Rationale for this change This is continuation of work in the FFI crate to expose useful traits. We currently have FFI catalog, schema, and table providers. The next layer up in the heirarchy is the catalog provider list. ## What changes are included in this PR? - Implement FFI_CatalogProviderList - Add unit tests and integration tests - Minor rearrangement of integration test for catalog ## Are these changes tested? Yes, included in PR. ## Are there any user-facing changes? This is only new addition to the FFI crate. No existing code is modified except making one data member pub(crate)
github-actions
bot
added
documentation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Which issue does this PR close?
This is related to #18843
Rationale for this change
This is a pure addition that users would like to use and it barely missed the cutoff for 51.0.0
What changes are included in this PR?
Adds in
FFI_CatalogProviderListAre these changes tested?
Yes, and already in
mainAre there any user-facing changes?
No, pure addition