DRILL-8239: Convert JSON UDF to EVF - #2567
Conversation
cgivre
changed the title
fc1a320 to
8dd00f8
Compare
cgivre
changed the title
63023ef to
5ab0489
Compare
87a0d60 to
25c9069
Compare
6b65419 to
3bfb116
Compare
Rewrite the convert_fromJSON UDFs to use the EVF JsonLoader (ResultSetLoader)
instead of the legacy JsonReader, mirroring the HTTP storage plugin UDFs.
JsonConverterUtils builds the loader from either the system JSON options or the
explicit allTextMode/readNumbersAsDouble arguments, and centralises the per-row
conversion.
To preserve the full convert_fromJSON contract, the EVF complex-writer support
in ProjectRecordBatch is extended:
* Multiple complex-writer functions per project list. addLoader now keeps a
list of (loader, output-column) pairs -- captured at codegen in
DrillComplexWriterFuncHolder -- so each loader's output lands in the column
reserved for it (fixes SELECT convert_from(a) m1, convert_from(b) m2 and
cases that project columns before/after the function).
* Top-level scalars and arrays. The UDF wraps each input value in a single
marker field so the record-oriented loader reads {scalar, array, object}
uniformly; ProjectRecordBatch unwraps that marker column by transferring it
directly (preserving the value's own type), and otherwise wraps the loader's
columns in a map (the HTTP-UDF behaviour).
* Per-output-batch lifecycle. Loaders are re-started before each batch, fixing
the "Unexpected state: HARVESTED" failure on multi-row/multi-batch input.
* Null/empty input writes an aligned (null) row so the loader row count matches
the surrounding batch.
3bfb116 to
7d3bfee
Compare
| import org.apache.drill.exec.physical.config.Project; | ||
| import org.apache.drill.exec.physical.resultSet.ResultSetLoader; | ||
| import org.apache.drill.exec.record.AbstractSingleRecordBatch; | ||
| import org.apache.drill.exec.record.MaterializedField; |
There was a problem hiding this comment.
nit: The imports for MaterializedField and JsonConverterUtils break the alphabetical ordering of the other imports.
There was a problem hiding this comment.
Fixed in c5b7424 — JsonConverterUtils moved up next to the other org.apache.drill.exec.expr imports and MaterializedField moved after BatchSchema.
| rsLoader.setTargetRowCount(count); | ||
| } else if (!CollectionUtils.isEmpty(rsLoaders)) { | ||
| for (ResultSetLoader loader : rsLoaders) { | ||
| loader.setTargetRowCount(count); |
There was a problem hiding this comment.
What use case would actually cause this loop body to execute?
In my opinion, the loop body calling setTargetRowCount in ProjectRecordBatch.setValueCount() is dead code.
Two cases exist:
With a ComplexWriter function (convert_fromJSON, split, etc.): addComplexField() is called → complexWriters is non-null( initComplexWriters()->projectBatch.complexWriters = new ArrayList<>();) → complexWriters != null is true → enters the first branch, else if is short‑circuited.
Without any ComplexWriter function (e.g. select *): complexWriters stays null → complexWriters != null is false → the else if condition is evaluated. But addLoader() is also not called → rsLoaders is null → !CollectionUtils.isEmpty(rsLoaders) is false → the loop body is not entered.
There was a problem hiding this comment.
I didn't originally write this, so I'm not 100% sure, but if I remember correctly, this was necessary for multi-row queries.
There was a problem hiding this comment.
In my opinion, Multi-row queries don't change the branch logic here. Whether complexWriters is null is decided at setup time (by addComplexField() → initComplexWriters()), not per-batch.
Are you referring to a multi‑row scenario like following example? Query reads 10,000 rows from test_multirow.json
and, for each row, concatenates id and name into a JSON string, then converts it with convert_fromJSON. Does that match what you meant by "multi‑row queries"?
{"id":6,"name":"user_6","value":60}
apache drill> SELECT
2..semicolon> convert_fromJSON(CONCAT('{"nested_id":', CAST(id AS VARCHAR), ',"orig_name":"', name, '"}')) AS js
3..semicolon> FROM
4..semicolon> dfs.test.`test_multirow.json`;
There was a problem hiding this comment.
You're right, and I was wrong in my earlier answer — thanks for pushing back with the analysis.
The deciding detail is in ProjectBatchBuilder.addComplexField(): it calls initComplexWriters() unconditionally, for EVF functions as well as legacy ComplexWriter ones. So whenever rsLoaders is non-empty, complexWriters has already been set to a (possibly empty) list, complexWriters != null is true, and the else if can never be reached. Multi-row queries don't change that — as you say, the branch is decided at setup time, not per batch.
Removed the branch in c5b7424. The loaders don't need a row count there anyway: their row count follows the rows the generated function wrote, and harvestLoaders() runs immediately after setValueCount(). I also made the complexWriters checks use CollectionUtils.isEmpty() consistently, since != null vs isEmpty() was what made the branch selection confusing in the first place.
To convince myself the loader's default row count limit wasn't quietly relying on that call, I added testConvertFromJsonBatchLargerThanLoaderRowLimit — a conversion over DEFAULT_ROW_COUNT * 2 + 17 rows. It passes with the branch removed.
|
|
||
| /** | ||
| * Wraps the raw input value in a single-field JSON object ({@code {"json": <value>}}) | ||
| * so that the record-oriented JSON loader accepts top-level scalars and arrays. |
There was a problem hiding this comment.
nit: getWrappedJsonStream Javadoc says {"json": } but the actual field is drill_json_value_wrapper (WRAP_FIELD).
There was a problem hiding this comment.
Good catch — the Javadoc predated the rename. Updated in c5b7424 to name the real wrapper field and link to WRAP_FIELD.
- Remove the unreachable result set loader branch in setValueCount(). Every complex-writer function goes through ProjectBatchBuilder.addComplexField(), which always initialises complexWriters, so the else-if could never run. Loaders need no row count here: the count follows the rows the generated function wrote and the batch is harvested immediately afterwards. - Use CollectionUtils.isEmpty() for complexWriters consistently. - Restore alphabetical import order in ProjectRecordBatch. - Correct the getWrappedJsonStream Javadoc to name the real wrapper field. - Add a test covering a JSON conversion over more rows than the loader's default row count limit.
3 participants
DRILL-8239: Convert JSON UDF to EVF
Description
This PR switches the
convert_fromJSONUDFs over to the EVF JsonLoader instead of the old JsonReader. The bulk of the work was in ProjectRecordBatch, which had to learn how to handle more than one complex-writer function per query, top-level JSON scalars and arrays, and multi-row batches — none of which the existing EVF path supported. Tests that broke along the way (TestComplexTypeWriter, TestConvertFunctions, the HTTP UDFs, and others) all pass again.Documentation
No user facing changes.
Testing
Ran unit tests.