fix: validate CSV expression function options like Spark - #2255
Open
davidlghellin wants to merge 12 commits into
Open
fix: validate CSV expression function options like Spark#2255davidlghellin wants to merge 12 commits into
davidlghellin wants to merge 12 commits into
Conversation
`from_csv`, `to_csv` and `schema_of_csv` silently ignored every option they did
not themselves use, so a bad value was accepted where Spark rejects the call:
`from_csv('1', 'a INT', map('header', 'garbage'))` returns `{1}` in Sail and
fails in Spark.
Each `from_map` was an allowlist — it pulled out the keys it knew and dropped
the rest. Spark instead builds `CSVOptions` eagerly, so every option is parsed
as soon as the map is read, even by a function that never uses the resulting
value: `from_csv` rejects a bad `quoteAll` and `to_csv` rejects a bad `header`.
Validation now lives in one shared place that every function routes through.
Each behaviour below was measured against Spark 4.1.1 rather than inferred:
- boolean options, including `multiLine`, which Spark reads with Scala's
`String.toBoolean` and therefore reports differently to the other seven
- single-character options, sized with Java's UTF-16 `String.length`: an emoji
is two units and is rejected, `ñ` is one and is accepted
- an empty `sep`, and an empty `lineSep`, which only the writer rejects
- numeric and enum options, and `DROPMALFORMED`, which only `from_csv` rejects
- NULL option values, which Spark rejects even under a key it does not know
- option keys, matched case-insensitively and reported by their canonical name
The three copies of the map lookup had each drifted apart: one compared keys
exactly, one guarded a NULL value and another did not, one built its default
eagerly. They are now a single `find_option`, which makes the NULL bug
unrepresentable rather than merely guarded.
Ten scenarios are tagged `@sail-bug` for divergences this does not fix: the
parse mode (Sail reads no `mode`, so it always aborts where Spark's default
PERMISSIVE nulls the failed field and keeps the good ones), the extra `.trim()`
on every token, and `schema_of_csv` ignoring `dateFormat` and a time-less
`timestampFormat`. They pass against the JVM and fail against Sail, so CI
reports an XPASS the day any of them is fixed.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns Sail’s from_csv, to_csv, and schema_of_csv behavior with Spark by eagerly validating CSV option maps (including options the specific function doesn’t otherwise use), and centralizing the option lookup/validation logic to prevent drift across implementations.
Changes:
- Added a shared CSV options module that provides case-insensitive option lookup and Spark-compatible eager validation.
- Updated
from_csv,to_csv, andschema_of_csvto route through the shared validation/lookup helpers. - Added extensive BDD coverage for Spark parity across boolean, char, numeric, enum, separator, and NULL option-value handling.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/pysail/tests/spark/function/features/csv_options.feature | Adds Spark-parity BDD scenarios covering eager option validation and known divergences (xfail). |
| crates/sail-function/src/scalar/csv/spark_to_csv.rs | Routes to_csv option parsing through shared validate_options / find_option / parse_bool_option. |
| crates/sail-function/src/scalar/csv/spark_from_csv.rs | Routes from_csv option parsing through shared validate_options / find_option. |
| crates/sail-function/src/scalar/csv/schema_of_csv.rs | Adds shared validation and makes option key matching case-insensitive when applying known options. |
| crates/sail-function/src/scalar/csv/options.rs | Introduces shared Spark-like CSV option lookup and eager validation logic. |
| crates/sail-function/src/scalar/csv/mod.rs | Registers the new options module. |
Comment on lines
+63
to
+75
| fn entry_columns(map: &MapArray) -> Option<(&StringArray, &StringArray)> { | ||
| let keys = map | ||
| .entries() | ||
| .column_by_name(SAIL_MAP_KEY_FIELD_NAME)? | ||
| .as_any() | ||
| .downcast_ref::<StringArray>()?; | ||
| let values = map | ||
| .entries() | ||
| .column_by_name(SAIL_MAP_VALUE_FIELD_NAME)? | ||
| .as_any() | ||
| .downcast_ref::<StringArray>()?; | ||
| Some((keys, values)) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Good catch — confirmed: make_scalar_function chains Hint::Pad, so the literal map is expanded to one copy per row and validate_options was walking the same options once per row.
Contributor
Author
|
This is the CSV slice of a systemic gap: from_json/to_json and from_xml/to_xml build their options the same allowlist way and drop bad values just as silently. They are deliberately left out — each family is its own PR, with its own JVM sweep, since the option sets and error messages differ. |
Spark 3.5.7 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
Spark 4.1.1 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests(truncated) |
Ibis Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #2255 +/- ##
==========================================
- Coverage 79.35% 78.18% -1.18%
==========================================
Files 916 917 +1
Lines 176215 176280 +65
==========================================
- Hits 139844 137831 -2013
- Misses 36371 38449 +2078
*This pull request uses carry forward flags. Click here to find out more.
... and 61 files with indirect coverage changes 🚀 New features to boost your workflow:
|
shehabgamin
added
run spark tests
Contributor
|
@davidlghellin I had Codex 5.6 ultra do a first pass. Let me know if any of this is relevant or not. If it is, please re-ping me when ready for re-review! |
Contributor
Author
Thanks for the thorough review — and for running Codex on it. I went through all 8 findings against Spark JVM, measuring each one rather than trusting the diff. Answering in your order: 1. (P1) Validator omits options Spark validates eagerly 2. (P1) Eager validation breaks Spark's null propagation (the regression) 3. (P1) Duplicate keys / casing / aliases don't follow the effective-map semantics 4. (P2) Delimiter parsing doesn't implement Spark's escape grammar ( 5. (P2) samplingRatio uses Rust's float grammar instead of Java's 6. (P2) lineSep length validation missing for to_csv 7. (P2) Non-literal / non-string option maps bypass Spark's analysis rules 8. (P3) ASCII-only case folding differs from Spark for Unicode Summary: 2, 3, 6, 8 fully fixed · 1 partially · 4, 5, 7 deferred — each deferred item is tracked with a red |
shehabgamin
reviewed
Jul 23, 2026
shehabgamin
left a comment
shehabgamin
left a comment
Contributor
There was a problem hiding this comment.
Well done! I only have a couple of comments
| } | ||
| for option in INT_OPTIONS { | ||
| if let Some(value) = find_option(map, option) | ||
| && value.parse::<i32>().is_err() |
Contributor
There was a problem hiding this comment.
Is it possible to use Java/Scala numeric parsing here?
Contributor
Author
There was a problem hiding this comment.
Good catch — I measured it, and there is a real divergence, though a narrow one:
| value | Java | Rust parse::<i32>() |
|---|---|---|
٥ (Arabic-Indic), १२ (Devanagari) |
accepts | rejects |
" 5 ", "5 " |
rejects | rejects ✓ |
+5 |
accepts | accepts ✓ |
2147483648 (overflow) |
rejects | rejects ✓ |
Whitespace, sign and overflow already match. The only gap is Unicode decimal digits: Java's Integer.parseInt goes through Character.digit (Unicode Nd), while Rust's parse is ASCII-only. Matching it isn't a one-liner — Rust's char::to_digit is ASCII-only too, so it needs an Nd table or a new dependency.
Added a test for it (@sail-bug, so CI tracks it):
@sail-bug
Scenario: from_csv accepts a Unicode decimal digit in an integer option
SELECT from_csv('1', 'a INT', map('maxColumns', '٥')) AS result
→ | {1} || /// not seen when every input row is NULL. Callers therefore invoke this only when the input has at | ||
| /// least one non-null row; the structural NULL-entry check ([`reject_null_entries`]) runs | ||
| /// unconditionally instead. Unknown keys are ignored, also matching Spark. | ||
| pub(super) fn validate_options(map: &MapArray, function: CsvFunction) -> Result<()> { |
Contributor
There was a problem hiding this comment.
I think Spark also validates extension, charset, time zone, compression codec, etc. But this can be followup work.
Contributor
Author
There was a problem hiding this comment.
Agreed — encoding/charset, timeZone and compression are tracked with @sail-bug scenarios so CI keeps an eye on them, and they're on the followup list.
One caveat on extension specifically: I measured it and it looks like a Spark quirk rather than validation we should mirror. It's a file-writer option (added by SPARK-50616 for the CSV DataSource writer), not an expression option. Passing it to to_csv accepts ab/abc/abcd but throws INTERNAL_ERROR on a non-letter value like a1 — and INTERNAL_ERROR is Spark's "this shouldn't happen" class, not a user-facing validation. So I'd lean toward leaving Sail ignoring it as an unknown option on the expression path, rather than replicating it. Happy to revisit if you'd rather match it.
Spark 3.5.9 Test ReportCommit Information
Test Summary
Test DetailsError CountsPassed Tests Diff(empty) Failed Tests |
Spark 4.2.0 Test ReportCommit Information
Test Summary
Test DetailsError Counts(truncated) Passed Tests Diff--- before.txt 2026-07-28 08:32:56.835377765 +0000
+++ after.txt 2026-07-28 08:32:57.440376818 +0000
@@ -1888,0 +1889 @@
+pyspark/sql/tests/connect/test_parity_functions.py::FunctionsParityTests::test_make_timestamp
@@ -1931,0 +1933 @@
+pyspark/sql/tests/connect/test_parity_functions.py::FunctionsParityTests::test_try_make_timestampFailed Tests(truncated) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
from_csv,to_csvandschema_of_csvsilently ignored every option they did not themselves use, so a bad value was accepted where Spark rejects the call:from_csv('1', 'a INT', map('header', 'garbage'))returns{1}in Sail and fails in Spark.Each
from_mapwas an allowlist — it pulled out the keys it knew and dropped the rest. Spark instead buildsCSVOptionseagerly, so every option is parsed as soon as the map is read, even by a function that never uses the resulting value:from_csvrejects a badquoteAllandto_csvrejects a badheader. Validation now lives in one shared place that every function routes through.Each behaviour below was measured against Spark 4.1.1 rather than inferred:
multiLine, which Spark reads with Scala'sString.toBooleanand therefore reports differently to the other sevenString.length: an emoji is two units and is rejected,ñis one and is acceptedsep, and an emptylineSep, which only the writer rejectsDROPMALFORMED, which onlyfrom_csvrejectsThe three copies of the map lookup had each drifted apart: one compared keys exactly, one guarded a NULL value and another did not, one built its default eagerly. They are now a single
find_option, which makes the NULL bug unrepresentable rather than merely guarded.Ten scenarios are tagged
@sail-bugfor divergences this does not fix: the parse mode (Sail reads nomode, so it always aborts where Spark's default PERMISSIVE nulls the failed field and keeps the good ones), the extra.trim()on every token, andschema_of_csvignoringdateFormatand a time-lesstimestampFormat. They pass against the JVM and fail against Sail, so CI reports an XPASS the day any of them is fixed.