Skip to content

feat: native randstr implementation compatible with Spark#5035

Merged
andygrove merged 4 commits into
apache:mainfrom
andygrove:feat/native-randstr
Jul 26, 2026
Merged

feat: native randstr implementation compatible with Spark#5035
andygrove merged 4 commits into
apache:mainfrom
andygrove:feat/native-randstr

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

There is no dedicated tracking issue; randstr was previously listed as planned (🔜) in the expression support guide.

Closes #.

Rationale for this change

Spark 4.0+ randstr(length[, seed]) fell back to Spark because Comet had no native implementation. Spark generates a random alphanumeric string per row via ExpressionImplUtils.randStr, seeding an XORShiftRandom with seed + partitionIndex per partition and drawing abs(rng.nextInt() % 62) per character (mapped onto 0-9/a-z/A-Z). Comet already ports that exact XORShiftRandom for native rand/randn, so randstr can run natively and produce results that match Spark bit for bit, keeping the enclosing operator on the native pipeline instead of falling back.

What changes are included in this PR?

This change was scaffolded with the project's implement-comet-expression skill (and audited with audit-comet-expression).

  • Native RandStrExpr (native/spark-expr/src/nondetermenistic_funcs/randstr.rs), modeled on the rand/uuid pattern: it keeps a per-partition XorShiftRandom in a Mutex so state advances across batches, and encodes each length-byte string into a pre-sized StringBuilder. Added XorShiftRandom::next_i32() (Java Random.nextInt()).
  • Proto RandStr { length, seed } message wired through the expression registry with a RandStrBuilder that combines the seed with the partition index (matching rand/randn/shuffle).
  • CometRandStr serde emitting the resolved literal length and seed. Because RandStr is a Spark 4.0+ class, the serde lives under spark-4.x and is registered via Spark4xCometExprShim.sparkVersionSpecificStringExpressions, so Spark 3.4/3.5 (which lack the class) still compile.
  • Support-status doc flipped from planned to supported, and a misc_funcs audit entry recording the cross-version findings.

The audit confirmed ExpressionImplUtils.randStr and the XORShiftRandom seeding are byte-identical across Spark 4.0.1 and 4.1.1, so no intra-4.x shim is needed. length and seed are required-foldable; Comet handles non-negative literal length with a literal seed and otherwise falls back to Spark (which also raises the negative-length error).

How are these changes tested?

  • Rust unit tests in randstr.rs: length and alphanumeric charset, zero length, determinism per (length, seed), and per-batch state continuity.
  • expressions/string/randstr.sql (Spark 4.0+, gated with MinSparkVersion: 4.0): seeded randstr(length, seed) asserts bit-for-bit equality with Spark across single and multiple rows, negative/zero/boundary seeds, lengths 0/1/12/64, and combined with other expressions; a negative length asserts the INVALID_PARAMETER_VALUE.LENGTH fallback; the no-seed randstr(length) form checks deterministic length and alphabet properties. Verified natively on Spark 4.1.2 (default); on Spark 3.5 the build compiles and the gated file is skipped.

…er-row UTF-8 scan

Move the literal-length/literal-seed/non-negative-length restrictions from convert
into getSupportLevel + getUnsupportedReasons so they surface in EXPLAIN and the
compatibility docs, matching the rand/randn serde pattern. Build each string in a
reused String buffer instead of validating a byte buffer per row.
@andygrove andygrove added this to the 1.0.0 milestone Jul 25, 2026

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @andygrove
Couple of nits

  • Capacity math num_rows * self.length — safe under 64-bit usize but consider checked_mul if adversarial huge length literals are ever a concern.

Test gaps

  • No golden-value Rust test. rand.rs has SPARK_SEED_42_FIRST_5; randstr.rs should have a SPARK_SEED_N_FIRST_M fixture generated from Spark's ExpressionImplUtils.randStr so byte-level Spark
    compatibility is asserted in-process (independent of the SQL-test Spark comparison, which is slow and only runs under a Spark-enabled profile).
  • No Int-vs-Long seed unit test. Serde does s.toLong sign extension; a test showing Literal(-1, IntegerType) and Literal(-1L, LongType) produce identical output would lock this in.
  • No explicit partition-index unit test. Only partition().into() addition is trusted; a small test that runs RandStrExpr::new(len, base_seed + p) and compares against expected Spark-side output for
    the same partition would validate the whole path.
  • SQL tests:
    • Missing a check that randstr(length, seed) output is stable across parquet.enable.dictionary and across split boundaries (add a ConfigMatrix comment header + a multi-partition table if feasible).
    • Missing a case with randstr under a filter / groupBy to confirm nondeterministic caching semantics (two calls in SELECT upper(randstr(6,7)), length(randstr(6,7)) exists — good — but no test with
      randstr in WHERE/HAVING).
    • Test for very large length (e.g., randstr(10_000_000, 0)) — not required, but a smoke test would catch capacity-math regressions.
  • Rust test_randstr_zero_length uses seed=42 — pin the empty output expected for a wider seed set (or drop the seed dimension entirely; it's irrelevant when length=0).

# Conflicts:
#	docs/source/user-guide/latest/expressions.md
Address review feedback on the randstr PR:

- Rust golden-value test asserting bit-for-bit equality with Spark 4.1.1
  across positive, zero, and negative seeds, independent of the SQL tests.
- Rust partition-index test exercising the seed + partition_index arithmetic.
- Broaden the zero-length test across a wider seed set (seed is irrelevant
  when length is zero).
- Rust large-length smoke test guarding the builder capacity math, and
  saturating_mul on the capacity hint to avoid overflow on huge lengths.
- SQL test asserting an Int seed and its Long literal produce identical
  output (serde toLong sign extension).
- SQL test running randstr through a native projection feeding a filter.
@andygrove

Copy link
Copy Markdown
Member Author

Thanks for the thorough review @comphead. I merged latest main and pushed the additional coverage. Golden values were captured from real Spark 4.1.1 ExpressionImplUtils.randStr.

Rust tests (randstr.rs)

  • test_randstr_matches_spark_golden: bit-for-bit fixtures for positive (42), zero (0), and negative (-1) seeds, asserted in-process independent of the SQL comparison. The negative-seed fixture also pins the Int -> Long sign extension.
  • test_randstr_partition_index_seed: exercises the base_seed + partition_index arithmetic by constructing RandStrExpr::new(len, base + p) and comparing against the seed-42 golden output.
  • test_randstr_zero_length: broadened across a wider seed set since the seed is irrelevant at length 0.
  • test_randstr_large_length: capacity-math smoke test, plus I switched the pre-size hint to saturating_mul so a huge length literal cannot overflow it.

SQL tests (randstr.sql)

  • Int vs Long seed: randstr(8, -1) = randstr(8, -1L) runs natively and returns true. Note I used a -1L literal rather than cast(-1 as bigint): the suite disables ConstantFolding, so the cast stays a non-literal and randstr falls back.
  • randstr feeding a filter: computed in a native projection, then filtered on the projected column.

A few I handled differently, happy to revisit:

  • parquet.enable.dictionary ConfigMatrix: skipped. randstr ignores its input columns entirely, so parquet encoding cannot change the output, and the SQL-test guide asks not to add ConfigMatrix speculatively.
  • Multi-partition split-boundary SQL test: the partition-index seeding is now covered directly by the Rust test above. Comparing a per-partition-seeded nondeterministic expression across the Comet vs Spark scans is fragile (it relies on both engines producing identical file partitioning), so I kept the assertion at the unit level.
  • randstr in WHERE/HAVING: a nondeterministic predicate in a Filter is not accelerated (falls back to Spark), so a direct WHERE randstr(...) would fail the native assertion. The projection-fed filter above keeps randstr native while still testing the filtering context. Spark also restricts nondeterministic expressions in grouping/having.

@andygrove
andygrove merged commit 8a9473c into apache:main Jul 26, 2026
136 of 138 checks passed
@andygrove
andygrove deleted the feat/native-randstr branch July 26, 2026 17:07
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.

2 participants