Skip to content

Commit 0921345

Browse files
committed
chore: replace deprecated object_store Path::child with join
object_store 0.13 deprecates Path::child in favor of .join()/.clone().join() (exact behavioral equivalent: single percent-encoded segment; join takes self by value). Fixes 8 call sites that would fail CI under -D warnings. Also adds UPGRADE-NOTES capturing the to_timestamp upstream-kept decision and the encrypted_parquet feature-flag finding.
1 parent ff72563 commit 0921345

5 files changed

Lines changed: 128 additions & 6 deletions

File tree

UPGRADE-NOTES-df53.1-arrow58.3.md

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
# Upgrade notes — DataFusion 50.1→53.1 / arrow 56.2→58.3
2+
3+
InfluxData-internal notes for the `upgrade-df-ver5310-a` branch. Records decisions and
4+
behavioral deltas that are **not** obvious from the code or commit messages. Keep this file
5+
updated for any further changes; **delete it before any PR to apache** (influx-internal scratch).
6+
7+
Versions: DataFusion **53.1.0**, arrow/parquet **58.3.0** (skip-pinned ahead of 53.1's declared
8+
58.0, by request), sqlparser 0.61, object_store 0.13.x, MSRV 1.88, edition 2024.
9+
10+
---
11+
12+
## 1. `to_timestamp` execution-timezone behavior — UPSTREAM KEPT (revert deliberately skipped)
13+
14+
**Decision (2026-05-27, Paul Dix): keep upstream 53.1 behavior. Do NOT re-apply the revert.**
15+
Documented here because it may matter later.
16+
17+
### What this is
18+
The prior 52.5 patch set carried influx commit `bb43b6890`,
19+
*Revert "Respect execution timezone in to_timestamp and related functions (#19078)"*.
20+
Upstream #19078 (landed 2026-01-05, in 53.1.0) changed the `to_timestamp*()` family so that
21+
**naïve (timezone-free) timestamp strings are interpreted in the configured
22+
`datafusion.execution.time_zone`** (UTC only when that option is `None`), and results carry the
23+
execution timezone. Before #19078, naïve strings were always interpreted as **UTC**.
24+
25+
The 52.5 revert restored the always-UTC behavior. During the 53.1 upgrade the revert was **skipped**
26+
(it conflicts in ~14 regions against 53.1's reworked `to_timestamp.rs` / `common.rs` / `mod.rs` /
27+
`to_date.rs` / `to_unixtime.rs` / `macros.rs`, and 53.1 also added Float16/Float32/Decimal numeric
28+
input support via #19663 that must not be lost).
29+
30+
### Current state on this branch
31+
- Runs **upstream 53.1 behavior**: naïve `to_timestamp('...')` is interpreted in
32+
`datafusion.execution.time_zone`.
33+
- `datafusion/sqllogictest/test_files/to_timestamp_timezone.slt` is present and **passing**.
34+
- The upstream implementation bug in #19078 (#20223, scalar-float broadcast) was fixed by #20224
35+
on 2026-02-12, which is in 53.1 — so there is no outstanding correctness bug to work around.
36+
37+
### Why this is safe to keep (and the risk if assumptions change)
38+
This is a no-op for InfluxDB **as long as IOx does not set a non-UTC
39+
`datafusion.execution.time_zone`**. If any future IOx code path sets a non-UTC execution timezone,
40+
this upstream behavior will **silently shift parsed naïve timestamps** away from UTC — a correctness
41+
change for InfluxDB queries. **If that ever happens, re-derive the revert against 53.1** (force
42+
naïve input → UTC while preserving 53.1's numeric-type support); do not replay the 52.5-era diff.
43+
Touch points if re-deriving: `to_timestamp.rs`, `common.rs`, `mod.rs`, `to_date.rs`,
44+
`to_unixtime.rs`, `macros.rs`; reconcile `to_timestamp_timezone.slt` and
45+
`datetime/timestamps.slt`.
46+
47+
---
48+
49+
## 2. `encrypted_parquet.slt` — NOT an arrow 58.3 regression (false alarm in the original handoff)
50+
51+
**Resolution (2026-05-27): no code change, no test edit, no version pin-back needed.**
52+
53+
The original handoff flagged this test as failing due to a "genuine upstream change between parquet
54+
58.0 and 58.3." That diagnosis was **incorrect**. Root cause: the test was run **without** the
55+
`parquet_encryption` feature.
56+
57+
### What actually happens
58+
Parquet encryption (read *and* write) is gated behind `#[cfg(feature = "parquet_encryption")]`.
59+
With the feature **off**, the `format.crypto.*` table options are silently ignored, so the
60+
"encrypted" files are written as **plaintext** (footer magic `PAR1`, plaintext schema/column names
61+
on disk). The final step of the test then reads those plaintext files without keys and **succeeds**,
62+
so the `query error … decryption properties were not provided` assertion fails with
63+
"expected to fail, but actually succeed."
64+
65+
This is independent of the arrow/parquet version — the feature gate behaves identically under 58.0
66+
and 58.3. (Verified by reasoning about the `cfg` gate; the on-disk files confirm plaintext output
67+
when the feature is off.)
68+
69+
### How to run it correctly
70+
```bash
71+
cargo test -p datafusion-sqllogictest --test sqllogictests --features parquet_encryption -- encrypted_parquet
72+
```
73+
With the feature **on**: files are written with an encrypted footer (magic `PARE`, no plaintext
74+
column names on disk), the keyless read errors as expected, and the test **passes**. Verified on
75+
this branch under arrow/parquet 58.3.
76+
77+
**Operational note:** the bare `cargo test -p datafusion-sqllogictest --test sqllogictests` command
78+
fails on `encrypted_parquet.slt` on *any* build without the feature (including pristine 53.1 base).
79+
Always pass `--features parquet_encryption` when running the slt suite, or run the full
80+
`cargo test --workspace` (where feature unification across workspace members enables it).
81+
82+
---
83+
84+
## 3. `nth_value` / `first_value` per-kind signatures — DO NOT UNDO
85+
86+
The "improve OneOf signature diagnostics" commit backports apache #21032 (not in 53.1). It gives
87+
`nth_value`/`first_value` **per-kind** signatures (`Signature::any(1)` / `Signature::any(2)`) rather
88+
than 53.1's `one_of([Nullary, Any(1), Any(2)])`. Reverting to `one_of` re-breaks 5 `errors.slt`
89+
cases (e.g. `one_of` wrongly lets `first_value(c5, 2)` succeed). **Keep per-kind.**
90+
91+
---
92+
93+
## 4. Pre-existing upstream test (not our regression)
94+
95+
`datafusion-expr``expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias` is flagged in the
96+
handoff as failing on the pristine 53.1.0 tag (feature/env-dependent), unrelated to our patches or
97+
the arrow bump. It did **not** surface in the full `cargo test --workspace` run on this branch
98+
(2026-05-27, exit 0, 0 failures across all suites). Leave it; flag upstream if it recurs.
99+
100+
---
101+
102+
## 5. Verification status (2026-05-27)
103+
104+
- `cargo test --workspace`**PASS** (exit 0, 0 failures across all suites).
105+
- `encrypted_parquet.slt` with `--features parquet_encryption`**PASS**.
106+
- `cargo clippy --workspace --all-targets -- -D warnings`**PASS** (exit 0) after the fix below.
107+
- **Fixed:** 8 `object_store::path::Path::child` deprecation warnings (object_store 0.13 bump)
108+
that would fail CI's `-D warnings`. Replaced with the maintainer-sanctioned `.join()` /
109+
`.clone().join()` (exact behavioral equivalent — single percent-encoded segment; `join` takes
110+
`self` by value so a `.clone()` is added where the receiver was borrowed). Sites:
111+
`datasource/src/write/demux.rs` (3), `datasource/src/url.rs` (1 line, 2 calls),
112+
`core/tests/sql/path_partition.rs` (1), `core/src/datasource/physical_plan/parquet.rs` (1).
113+
Affected tests re-run and green (datasource lib, `test_prefix_path`, `path_partition`,
114+
`parquet_exec_with_error`).
115+
116+
## 6. Finishing checklist
117+
- [x] §1 to_timestamp revert — resolved: keep upstream, documented above.
118+
- [x] §2 encrypted_parquet — resolved: false alarm, run with `--features parquet_encryption`.
119+
- [x] Full `cargo test --workspace` — green.
120+
- [x] `cargo clippy --workspace --all-targets -- -D warnings` clean (fixed 8 `Path::child` deprecations).
121+
- [ ] Delete `HANDOFF-df-upgrade-ver5310.md` and this `UPGRADE-NOTES-*.md` before any PR to apache.

datafusion/core/src/datasource/physical_plan/parquet.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1703,7 +1703,7 @@ mod tests {
17031703
let state = session_ctx.state();
17041704
let location = Path::from_filesystem_path(".")
17051705
.unwrap()
1706-
.child("invalid.parquet");
1706+
.join("invalid.parquet");
17071707

17081708
let partitioned_file = PartitionedFile::new_from_meta(ObjectMeta {
17091709
location,

datafusion/core/tests/sql/path_partition.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ impl ObjectStore for MirroringObjectStore {
766766
};
767767

768768
if parts.next().is_some() {
769-
common_prefixes.insert(prefix.child(common_prefix));
769+
common_prefixes.insert(prefix.clone().join(common_prefix));
770770
} else {
771771
let object = ObjectMeta {
772772
location: k.clone(),

datafusion/datasource/src/url.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -536,7 +536,7 @@ mod tests {
536536
let root = root.to_string_lossy();
537537

538538
let url = ListingTableUrl::parse(root).unwrap();
539-
let child = url.prefix.child("partition").child("file");
539+
let child = url.prefix.clone().join("partition").join("file");
540540

541541
let prefix: Vec<_> = url.strip_prefix(&child).unwrap().collect();
542542
assert_eq!(prefix, vec!["partition", "file"]);

datafusion/datasource/src/write/demux.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,8 @@ fn generate_file_path(
260260
if !single_file_output {
261261
base_output_path
262262
.prefix()
263-
.child(format!("{write_id}_{part_idx}.{file_extension}"))
263+
.clone()
264+
.join(format!("{write_id}_{part_idx}.{file_extension}"))
264265
} else {
265266
base_output_path.prefix().to_owned()
266267
}
@@ -588,8 +589,8 @@ fn compute_hive_style_file_path(
588589
) -> Path {
589590
let mut file_path = base_output_path.prefix().clone();
590591
for j in 0..part_key.len() {
591-
file_path = file_path.child(format!("{}={}", partition_by[j].0, part_key[j]));
592+
file_path = file_path.join(format!("{}={}", partition_by[j].0, part_key[j]));
592593
}
593594

594-
file_path.child(format!("{write_id}.{file_extension}"))
595+
file_path.join(format!("{write_id}.{file_extension}"))
595596
}

0 commit comments

Comments
 (0)