chore: deprecate record_batch macro in favor of upstream one#23295
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
| arrow-schema = { workspace = true } | ||
| datafusion = { workspace = true } | ||
| datafusion-ffi = { workspace = true } | ||
| ffi_module_interface = { path = "../ffi_module_interface" } | ||
|
|
||
| [package.metadata.cargo-machete] | ||
| ignored = ["arrow-schema"] |
There was a problem hiding this comment.
this seems pretty unfortunate, that its kind of a hidden dependency of the macro 🙁
i suppose we cant do much about it unless we fix it upstream (and even then would require arrow-array to reexport arrow-schema i think?)
There was a problem hiding this comment.
Yes, I was kinda surprised this was required as well. If you think this is a blocker I can close and just leave record_batch! as it is I'm not sure what is best
There was a problem hiding this comment.
yeah i think it's a bit awkward to need to import two dependencies in order to use this macro 🤔
There was a problem hiding this comment.
Using use arrow::datatypes as arrow_schema; import fixed the issue with this there is no need for adding these imports and machete ignores. Can you check again @Jefffrey ?
There was a problem hiding this comment.
this does solve the machete issue, though i still worry for any downstream users of the df version of the macro this would be a non-obvious fix 🤔
|
I've deleted the branch and closed it while cleaning up branches sorry |
507510e to
5786f7d
Compare
alamb
left a comment
There was a problem hiding this comment.
This PR looks good to me now. Am I missing anything here? I don't see any sort of cargo dependency shenanigans
When I've opened the PR I've added |
I am trying to figure out if there is a still a problem we need to fix --- like for example should we re-export the |
|
Thank you for this PR |
|
I think #23400 happened since every check passed but merge queue removed it |
|
I'll just throw it back in the queue! |
|
🤔 |
…23295) ## Which issue does this PR close? - Closes apache#13037. ## Rationale for this change This was a long standing thing in the backlog. record_batch! was added in arrow; however, it did not support vectors. Then I've created a PR in arrow a while ago apache/arrow-rs#9522. Once that was released this macro can be deprecated ## What changes are included in this PR? Mark record_batch! macro deprecated and use arrow one in relevant places. ## Are these changes tested? Yes ## Are there any user-facing changes? Users will see a deprecation notice if they are using this macro
Which issue does this PR close?
Rationale for this change
This was a long standing thing in the backlog. record_batch! was added in arrow; however, it did not support vectors. Then I've created a PR in arrow a while ago apache/arrow-rs#9522. Once that was released this macro can be deprecated
What changes are included in this PR?
Mark record_batch! macro deprecated and use arrow one in relevant places.
Are these changes tested?
Yes
Are there any user-facing changes?
Users will see a deprecation notice if they are using this macro