Skip to content

fix: raise clear parse error for invalid derivation expressions#222

Open
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:fix/derivation-expression-parse-error
Open

fix: raise clear parse error for invalid derivation expressions#222
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:fix/derivation-expression-parse-error

Conversation

@nielspardon

Copy link
Copy Markdown
Member

Problem

evaluate() builds the ANTLR parser in _parse() but installs no error-raising listener. On invalid syntax, ANTLR's default error recovery prints a warning to stderr and returns a partial parse tree containing None children, which _evaluate then walks into — surfacing as the confusing Exception: Unknown token type <class 'NoneType'>.

This is easy to hit with any precision-parameterized type used without its required argument, e.g. evaluate("interval_day"), evaluate("precision_timestamp"), evaluate("decimal"). Per the type grammar all of decimal, interval_day, interval_compound, precision_time, precision_timestamp, and precision_timestamp_tz require explicit parameters, so these are genuine syntax errors.

Fix

Attach a custom ANTLR error listener to both the lexer and parser in _parse() (after removing the default listeners, which also silences the stray stderr warnings). On any syntax error it raises a dedicated DerivationExpressionParseError naming the offending input, chaining the underlying ANTLR exception:

DerivationExpressionParseError: Could not parse derivation expression 'interval_day' at line 1:12: mismatched input '<EOF>' expecting {'<', '?'}

Fallout

Enabling strict parsing surfaced that test_nstruct_simple / test_nstruct_nested were only passing because of the silent recovery this change removes: they used nStruct<a i32, b i32>, but the grammar (substrait-antlr 0.96.0) requires colons — nStruct<a: i32, b: i32>. ANTLR had been inserting the missing : and continuing. Updated those tests to the valid colon syntax; the asserted results are unchanged.

Testing

Added a parametrized test covering every precision-parameterized type used without arguments, plus a general malformed-input case. pytest — 519 passed, 30 skipped; ruff check and ruff format --check clean.

Fixes #216.

🤖 Generated with AI

## Problem

`evaluate()` builds the ANTLR parser in `_parse()` but installs no
error-raising listener. On invalid syntax, ANTLR's default error recovery
prints a warning to stderr and returns a **partial** parse tree containing
`None` children, which `_evaluate` then walks into — surfacing as the
confusing `Exception: Unknown token type <class 'NoneType'>`.

This is easy to hit with any precision-parameterized type used without its
required argument, e.g. `evaluate("interval_day")`,
`evaluate("precision_timestamp")`, `evaluate("decimal")`. Per the type
grammar all of `decimal`, `interval_day`, `interval_compound`,
`precision_time`, `precision_timestamp`, and `precision_timestamp_tz`
require explicit parameters, so these are genuine syntax errors.

## Fix

Attach a custom ANTLR error listener to both the lexer and parser in
`_parse()` (after removing the default listeners, which also silences the
stray stderr warnings). On any syntax error it raises a dedicated
`DerivationExpressionParseError` naming the offending input, chaining the
underlying ANTLR exception:

```
DerivationExpressionParseError: Could not parse derivation expression
'interval_day' at line 1:12: mismatched input '<EOF>' expecting {'<', '?'}
```

## Fallout

Enabling strict parsing surfaced that `test_nstruct_simple` /
`test_nstruct_nested` were only passing because of the silent recovery this
change removes: they used `nStruct<a i32, b i32>`, but the grammar
(substrait-antlr 0.96.0) requires colons — `nStruct<a: i32, b: i32>`. ANTLR
had been inserting the missing `:` and continuing. Updated those tests to
the valid colon syntax; the asserted results are unchanged.

## Testing

Added a parametrized test covering every precision-parameterized type used
without arguments, plus a general malformed-input case. `pytest` — 519
passed, 30 skipped; `ruff check` and `ruff format --check` clean.

Fixes substrait-io#216.

🤖 Generated with AI
@nielspardon
nielspardon force-pushed the fix/derivation-expression-parse-error branch from 51ce39a to 1a43573 Compare July 16, 2026 17:44
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.

derivation_expression: invalid type syntax yields confusing "Unknown token type NoneType" instead of a parse error

1 participant