Repository navigation
fix(kernel): report untyped NULL columns as STRING - #569
vuanhphung wants to merge 3 commits into
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
The change looks correct, with one low concern: the metadata-driven NULL/VOID → STRING mapping is in place, but the Arrow-type fallback for native Null fields still reports NULL_TYPE. Value conversion isn't affected, since convertThriftValue treats NULL_TYPE and STRING_TYPE the same way, and the new tests cover both labels.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good. The NULL/VOID metadata mapping and the native Arrow Null mapping now both return STRING_TYPE, which matches Thrift. Nothing downstream relies on NULL_TYPE for value conversion: in convertThriftValue (lib/result/utils.ts), NULL_TYPE and STRING_TYPE both pass values through unchanged. The new parameterized tests cover all three paths and confirm the null values stay null.
Other findings
- ⚪ Nit — In
lib/kernel/KernelArrowIpc.ts, the lineif (DataType.isNull(arrowType)) return TTypeId.STRING_TYPE;now returns the same value as the finalreturn TTypeId.STRING_TYPE;fallthrough, so it no longer changes behavior. You could delete it, or keep it with a short comment so the Null→STRING mapping is stated explicitly. Either way, the result is the same.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
|
Addressed the latest review nit in d9af9b9: removed the redundant Arrow Null branch so it uses the existing STRING fallback. All nine backend round-trip tests, including the three NULL metadata cases, pass. This comment was generated with GitHub MCP. |
Kernel results for
SELECT NULLreportNULL_TYPEthroughgetSchema(), while Thrift andCAST(NULL AS STRING)reportSTRING_TYPE. Map retainedNULL/VOIDmetadata and native Arrow Null fields to STRING, with regression coverage preserving null values.Addresses DATATYPE-041 in #470. STATEMENT-023 remains open for a separate kernel fix.
Validated with 1,488 unit tests, build, lint, Prettier, and recorded-response replay using kernel 1.4.0 in default and async execution. TypeScript checking passes with
@databricks/sqlaliased to local sources; the default check encounters existing source/dist type duplication in a token-federation example.