mattcasters commented on PR #8304:
URL: https://github.com/apache/hop/pull/8304#issuecomment-5616508007
## Code Review of PR #8304: Formula Optimization
Great initiative! Bypassing Apache POI's HSSF/XSSF workbook and sheet
allocation for straightforward expressions is a huge win for one of Hop's most
heavily used transforms. The 10x–20x microbenchmark improvements demonstrate
how much overhead POI introduces for row-by-row formula evaluation.
Below are a few critical correctness and runtime failure issues, Excel/POI
semantic discrepancies, and performance suggestions to address before this can
safely merge.
---
### 1. Critical Bugs & Runtime Failure Risks
#### 1.1 `ArrayIndexOutOfBoundsException` on missing fields and bracketed
string literals
In `FastFormulaCompiler`:
```java
// FastFormulaCompiler.key():
for (String fieldName : fieldNames) {
key.append('|').append(fieldName);
key.append(':').append(rowMeta.getValueMeta(rowMeta.indexOfValue(fieldName)).getType());
}
// FastFormulaCompiler.eligibleTypes():
for (String fieldName : fieldNames) {
int type = rowMeta.getValueMeta(rowMeta.indexOfValue(fieldName)).getType();
if (!isFastType(type)) { return false; }
}
```
* **Issue:** If a formula references a field not present in `rowMeta` (e.g.
typos, missing input columns), `rowMeta.indexOfValue(fieldName)` returns `-1`.
Calling `rowMeta.getValueMeta(-1)` immediately throws
`ArrayIndexOutOfBoundsException: Index -1 out of bounds`.
* **String literal brackets:** In `Formula.java`, `fieldNames` is extracted
via `FormulaFieldsExtractor.getFormulaFieldList()`, which extracts any text
inside `[...]` without string literal awareness. A formula like:
```excel
IF([status] = "[ACTIVE]", 1, 0)
```
extracts `"ACTIVE"` as a field name. If `"ACTIVE"` is not a stream field,
initialization fails with `ArrayIndexOutOfBoundsException: -1` rather than
falling back to POI.
* **Recommendation:** Check `int idx = rowMeta.indexOfValue(fieldName); if
(idx < 0) return CompiledFormula.NOT_ELIGIBLE;` before attempting
`getValueMeta(idx)`.
---
#### 1.2 `null` in numeric fields crashes the pipeline with unhandled
`UnsupportedFormulaException`
In `FastFormulaEvaluator.toNumber()`:
```java
if (value == null || value == FastFormulaCompiler.NA) {
throw new UnsupportedFormulaException("Cannot use " + value + " as a
number");
}
```
* **Issue:** In Excel and Apache POI, blank/null cells in arithmetic
expressions (`[qty] * [price]` or `[amount] + 10`) are treated as `0` (`null +
10 = 10`, `null * 5 = 0`).
* In `FastFormulaEvaluator`, if an incoming row has a `null` value in a
numeric field:
1. `toNumber(null)` throws `UnsupportedFormulaException`.
2. Because the formula was marked eligible (`fastPath = true`) during
first row initialization, this exception occurs during row processing
(`eval(args)`).
3. `Formula.processRow()` catches `Exception` and diverts to error
handling or **aborts the pipeline**. There is **no runtime fallback to POI**.
* Real-world ETL data frequently contains nulls (e.g., from outer joins).
* **Recommendation:** In `toNumber()`, when `value == null` and `setNa` is
false, treat `null` as `0.0d` to match Excel/POI arithmetic behavior.
---
#### 1.3 `FastFormulaCompiler.NA` sentinel leaks into String concatenation
as `"java.lang.Object@..."`
In `FastFormulaCompiler`:
```java
public static final Object NA = new Object();
```
And in `FastFormulaEvaluator.TextValue`:
```java
private static String of(Object value) {
if (value == null) return "";
if (value instanceof Boolean ...) ...
if (value instanceof Number ...) ...
return String.valueOf(value);
}
```
* **Issue:** When `isSetNa()` is enabled and a null field is encountered,
`args[i]` is set to `FastFormulaCompiler.NA`. If that field is used in text
concatenation (`[prefix] & "-" & [comment]`), `TextValue.of(NA)` falls through
to `String.valueOf(NA)`, resulting in:
`"prefix-java.lang.Object@4f023fd2"`
* In Excel/POI, concatenating an `#N/A` error propagates `#N/A` (or produces
an error cell). It should not output the Java object hash.
---
#### 1.4 Unchecked exceptions in `compileUncached` escape unhandled
In `FastFormulaCompiler.compileUncached()`:
```java
try {
root = FastFormulaEvaluator.parse(resolvedFormula, fieldIndex);
} catch (UnsupportedFormulaException e) {
return CompiledFormula.NOT_ELIGIBLE;
}
```
* **Issue:** If the parser throws an unchecked exception such as
`NumberFormatException` (e.g., malformed exponential notation `1e-`),
`StringIndexOutOfBoundsException`, or `NullPointerException`, it bypasses this
catch block and crashes initialization instead of returning `NOT_ELIGIBLE`.
* **Recommendation:** Catch `Exception` (or `RuntimeException`) and return
`CompiledFormula.NOT_ELIGIBLE`.
---
### 2. Parity & Semantic Divergences with Excel / Apache POI
1. **`compareEqual` between Boolean and Number / String:**
```java
if (left instanceof Boolean || right instanceof Boolean) {
return toBoolean(left) == toBoolean(right);
}
```
* In Excel and POI, booleans and numbers are strictly distinct types:
`TRUE = 1` is `FALSE`, and `FALSE = 0` is `FALSE`. In `FastFormulaEvaluator`,
`toBoolean(1)` returns `true`, so `TRUE = 1` evaluates to `TRUE`.
* Additionally, if `left` is `Boolean` and `right` is `"Y"`,
`toBoolean("Y")` throws `UnsupportedFormulaException` at runtime instead of
returning `FALSE`.
2. **`compareEqual` between Number and String:**
* In Excel and POI, numbers and strings are never equal (`200 = "200"` is
`FALSE`). In `FastFormulaEvaluator`, it falls through to
`TextValue.of(left).equalsIgnoreCase(TextValue.of(right))`, which returns
`TRUE`.
3. **Non-standard logical operators (`&&`, `||`, `!`):**
* Excel formulas use `AND(...)`, `OR(...)`, `NOT(...)`. Excel does not
support `&&`, `||`, or `!` as logical operators (`!` is the sheet reference
separator).
* Introducing `&&` and `||` creates a dialect mismatch: formulas like
`[a] > 1 && [b] > 1` work on the fast path, but fail with
`FormulaParseException` in POI if the fast path is disabled or if an
unsupported function causes fallback.
4. **`IF` with 2 arguments:**
* In Excel and POI, the false branch is optional: `=IF([score] >= 60,
"Pass")` evaluates to `FALSE` when the condition is false.
`FastFormulaEvaluator` rejects this with `UnsupportedFormulaException("IF
requires 3 arguments")`.
5. **`TRIM` whitespace inconsistency:**
```java
if (text.indexOf(' ') < 0) {
return text.trim();
}
```
* If there are no spaces, `String.trim()` strips all whitespace (tabs,
newlines, control characters). If a space exists, the custom loop only
collapses/strips ASCII spaces (`' '`), leaving tabs and newlines intact.
---
### 3. Performance & Memory Optimizations
#### 3.1 Per-row `RowMeta.indexOfValue` lookups and per-row `Object[]`
allocations
In `Formula.java`:
```java
private Object[] buildFastArguments(List<String> fieldList, Object[]
sourceRow, boolean setNa) {
Object[] args = new Object[fieldList.size()];
for (int i = 0; i < fieldList.size(); i++) {
int fieldIndex = data.outputRowMeta.indexOfValue(fieldList.get(i));
Object value = fieldIndex < 0 ? null : sourceRow[fieldIndex];
args[i] = (value == null && setNa) ? FastFormulaCompiler.NA : value;
}
return args;
}
```
* `RowMeta.indexOfValue()` acquires a `ReentrantReadWriteLock.readLock()` on
every call. Calling it for every field of every formula on every single row
incurs repeated locking and string comparisons.
* In addition, allocating a new `Object[] args` array per formula per row
creates avoidable GC churn on large pipelines.
* **Optimization:** Pre-compute the field indices during `first` into an
`int[][] fastFieldIndices` array:
```java
int fieldIndex = fastFieldIndices[i][j];
Object value = sourceRow[fieldIndex];
```
#### 3.2 POI resources allocated even when all formulas use the fast path
In `Formula.java`:
```java
poi = IntStream.range(0, meta.getFormulas().size())
.mapToObj(it -> new FormulaPoi(this::logDebug))
.toArray(FormulaPoi[]::new);
formulaFieldLists = ...;
```
If every formula in the transform is eligible and compiled for the fast
path, pre-allocating `poi` and extracting `formulaFieldLists` can be skipped.
---
### 4. Hop Conventions & Configuration
1. **Hop Variables vs System Properties:**
* Configuration is currently controlled via JVM property
`-Dorg.apache.hop.pipeline.transforms.formula.fast.FastFormulaCompiler.enabled=false`.
In Hop, users configure options via `hop-config.json`, project settings, or
pipeline execution configurations. Exposing this via Hop environment variables
or `IVariables` would make it manageable in Hop GUI and `hop-run`.
* Also, `FastFormulaCompiler.enabled` is cached at class loading
(`private static volatile boolean enabled = enabledFromProperty();`), meaning
subsequent calls to `System.setProperty(...)` have no effect unless
`FastFormulaCompiler.setEnabled()` is called directly.
2. **Redundant `setNa` in Cache Key:**
* In `FastFormulaCompiler.key()`:
```java
key.append(setNa ? "na" : "plain").append('=').append(resolvedFormula);
```
`setNa` is not used during AST compilation in `compileUncached`; it is
only used at runtime in `buildFastArguments()`. Including `setNa` in the cache
key leads to duplicate cached ASTs.
--
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]
