kosiew commented on issue #24612:
URL: https://github.com/apache/datafusion/issues/24612#issuecomment-6096577405
@adriangb
All five sub-issues here are closed now (#24620, #24621, #24622, #24623,
#24624), and the three confirmed field drops are fixed on `main`. One part
hasn't landed: the "Public API to consider deprecating" section. None of the
three "safe to deprecate" accessors is deprecated yet:
- `CsvSource::newlines_in_values`: the plan was to deprecate it as part of
the #24609 fix, but #24609 closed without it.
- `AnalyzeExec::metric_categories`
- `ExplainExec::verbose`
I re-ran the downstream search (GitHub code search, plus grepping fresh
clones of the 8 repos from your survey and greptimedb, lance and
datafusion-federation):
- `CsvSource::newlines_in_values`: no callers.
- `AnalyzeExec::metric_categories`: no callers. Every hit was a vendored
copy of DataFusion or an unrelated identifier.
- `ExplainExec::verbose`: three callers, all copies of DataFusion's old
in-tree proto serializer (`ExplainExecNode { verbose: exec.verbose(), .. }`) in
projects that haven't been updated in a long time: `hetudb/hetu` (last push
2024-10), `suibianwanwank/hexa` (2024-05) and
`ep-infosec/33_apache_arrow-ballista` (2022-12).
I opened #26179 to deprecate `CsvSource::newlines_in_values`. It would also
switch the CSV encoder to read the field directly and add the 56.0.0
upgrade-guide entry. Before I touch the other two, could you confirm:
1. **`ExplainExec::verbose`: deprecate or keep?** It has a few (stale)
downstream callers, and your note argues read accessors should stay available
to external codecs, so my inclination is to keep it.
2. **Are the plan-node accessors in scope at all?** The issue lists
`AnalyzeExec::metric_categories` and `ExplainExec::verbose` as outside this
EPIC. Should they go in the new PR, or be left alone?
3. **Round-trip tests:** `datafusion/proto/tests/cases/plans/misc.rs` uses
`metric_categories()` and `ExplainExec::verbose()` to check that those fields
survive a round-trip. If we deprecate them, would you rather have
`#[expect(deprecated)]` in the tests, or assertions that compare
`Debug`/`fmt_as` output instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
