mattcasters commented on PR #8304:
URL: https://github.com/apache/hop/pull/8304#issuecomment-5714560145
## Follow-up review of PR #8304 (head `877c839a`)
Checked the “review feedback” commit against HEAD source, not the earlier
discussion. A lot of the 2026-09-10 items are actually fixed. The remaining
risk is still the same: the fast path is default-on, eligibility is decided on
the first row, and there is no per-row POI fallback once `fastPath=true`. A
leftover parity bug therefore either returns a different result than POI or
aborts the pipeline.
### Fixed in HEAD
- Missing / bracketed `[...]` fields no longer
`ArrayIndexOutOfBoundsException` at init; unknown fields return `NOT_ELIGIBLE`.
- Blank numeric cells are treated as `0.0d` in arithmetic, matching
Excel/POI.
- `#N/A` concatenation no longer emits `java.lang.Object@…`.
- `compileUncached` catches `RuntimeException` and returns `NOT_ELIGIBLE`
instead of crashing init.
- `=` is type-strict: `TRUE = 1` and `200 = "200"` are now `FALSE`.
- 2-argument `IF` is accepted (false branch is `FALSE`).
- Field indices are precomputed and `Object[]` args are reused per formula.
- POI workbooks are not allocated when every formula is on the fast path.
- `setNa` is no longer part of the compile cache key.
Those have compiler tests that would fail if the crash / null / `=` / `IF`
bugs came back.
### Leftovers and new issues
#### 1. `#N/A` still aborts outside concat
Concat now returns the `NA` sentinel (mapped to null). Every other use still
throws: `toNumber(NA)` / `toBoolean(NA)` / `TextValue.of(NA)` raise
`UnsupportedFormulaException` (`Cannot use java.lang.Object@…`). With “Set Null
to #N/A”, `[amount]+10` or `LEN([comment])` abort (or error-hop). POI evaluates
to `#N/A` and `Formula.getErrorValue` maps that to null.
**Suggestion:** propagate `FastFormulaCompiler.NA` through arithmetic,
boolean ops, `IF`/`AND`/`OR`, `LEN`, and `TRIM`; keep `ISNA` as the only
consumer. Add a `setNa=true` parity case for `[amount]+10` and
`[prefix]&[comment]`.
#### 2. TRIM now diverges the other way
Both TRIM paths now use `Character.isWhitespace`, so embedded tabs/newlines
become spaces. POI 5.5 is `arg.trim().replaceAll(" +", " ")` (Java `trim()` at
the ends, ASCII spaces only in the middle). `TRIM("a\t\tb")` is `"a b"` on the
fast path and `"a\t\tb"` on POI. The parity test only uses `" a b "`, which
matches both.
**Suggestion:** match POI (`text.trim().replaceAll(" +", " ")` or a loop
that only collapses `' '`). Add a parity row with embedded tabs/newlines.
#### 3. Ordered compares still stringify mixed types
`compareEqual` is type-strict, but `compareOrdered` still does
`TextValue.of(left).compareToIgnoreCase(...)` when both sides are not numbers.
POI ranks Bool > String > Number and does not convert across those types: `200
> "199"` is `FALSE` in POI (string > number) and `TRUE` here. `FALSE > "Z"` is
`TRUE` in Excel/POI and `FALSE` here (`"FALSE"` vs `"Z"`).
**Suggestion:** use the same type ranking as POI for `<` `>` `<=` `>=`.
Cover `200 > "199"` and a boolean-vs-string compare in
`FormulaFastPathParityTest`.
#### 4. `&&` / `||` / `!` are still a fast-only dialect
`parseOr` / `parseAnd` / `parseUnary` still accept `||`, `&&`, and `!`, and
`logicalOperatorsAreSupportedAsSymbols` still asserts they work. Excel/POI do
not have these operators (`!` is the sheet separator). `[a] > 1 && [b] > 1`
succeeds only on the fast path and throws `FormulaParseException` if the
property is off or the formula later falls back to POI.
**Suggestion:** reject `&&`, `||`, and `!` at parse time (`NOT_ELIGIBLE`) so
those formulas always go through POI. Flip the test to
`assertFalse(compiled.fastPath())`.
#### 5. Boolean arithmetic throws on the fast path (new)
`isFastType` includes `TYPE_BOOLEAN`, so `[flag]*10` and `TRUE+1` compile as
fast-path. `toNumber` then throws (`Cannot use true as a number`). POI
`BoolEval` is numeric (TRUE=1, FALSE=0), so the same formulas return 10 and 2.
Once marked eligible, the exception aborts the row instead of falling back.
**Suggestion:** coerce `Boolean` to `1.0d`/`0.0d` in `toNumber` (keep
equality type-strict). Add a parity case `[flag] * 10` with a boolean field.
#### 6. Enablement is still a class-load JVM property
The only control is
`-Dorg.apache.hop.pipeline.transforms.formula.fast.FastFormulaCompiler.enabled`,
default `true`, snapshotted at class load. `System.setProperty` after that
does nothing unless `setEnabled` is called. There is still no Hop variable,
`hop-config` key, or Formula dialog control, so a pipeline that hits one of the
remaining parity bugs cannot be switched back to POI from Hop GUI / hop-run
without a JVM flag.
**Suggestion:** read the flag from `IVariables` (and/or a transform option)
on `first`. A GUI checkbox is the Hop-shaped control if this stays inside
Formula rather than a separate transform.
#### 7. Parity test never asserts the fast path was taken
`FormulaFastPathParityTest` never asserts `compiled.fastPath()`. If compile
returns `NOT_ELIGIBLE`, both legs use POI and `assertRowsEqual` still passes —
so this test would not have failed for the old 3-arg-only `IF` or a TRIM
fallback. Tabs in TRIM, `setNa` arithmetic, mixed ordered compares, and boolean
arithmetic have no coverage on either side.
**Suggestion:** when `enabled` is true, assert the formula compiled to
`fastPath()`. Add POI-vs-fast rows for the leftover cases above.
Until the leftover parity / abort cases are closed, I would not treat the
fast path as a transparent POI subset.
--
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]
