zhangshenghang commented on PR #11216:
URL: https://github.com/apache/seatunnel/pull/11216#issuecomment-4890484554
Thanks for this PR — the core design (opt-in flag, default `false`,
PK/vector exclusion, E2E null round-trip assertions) is solid. I have a few
concerns to address before this can merge.
## Critical
**1. Default-config backward-compat break for null dynamic-field values**
In `MilvusSinkConverter#buildMilvusData`, the branch order changed: the
dynamic-field branch now runs *before* the null check. The old code threw
`FIELD_IS_NULL` (MILVUS-10) on any null value (including the dynamic field)
because the null check came first. With the new ordering, a null value in the
dynamic-field column is silently skipped (`if (value != null) {...} continue;`)
— even under the **default** config (`enable_dynamic_field=true`,
`enable_nullable_field=false`).
So a job that previously crashed on a row whose dynamic-field column was
null now silently writes that row with no dynamic data. The PR description
states "default value is false, so existing jobs keep the old behavior and
still fail on null values with MILVUS-10" — that's not accurate for this case,
and `skipsNullDynamicFieldBeforeNullValidation` codifies the changed behavior.
Per SeaTunnel's backward-compat policy, please either restore the throw when
`enable_nullable_field=false`, or document the change in
`docs/en/introduction/concepts/incompatible-changes.md`. (Note: when
`enable_dynamic_field=false` the behavior is preserved — the gap is
specifically the default `enable_dynamic_field=true`.)
## Important
**2. Partition key is not excluded from nullable**
`supportNullableField` excludes the primary key and vector types, but not
the partition key. `convertToFieldType` already receives `partitionKeyField`,
so the fix is one line. In Milvus 2.5 the partition key can't be nullable (it's
required for routing); with `enable_nullable_field=true` and a nullable
partition-key column, `createCollection` will fail with a confusing error
instead of a clean MILVUS-10. I'd suggest adding a null-safe
`!column.getName().equals(partitionKeyField)` check. No test covers this case
today.
**3. Docs don't state the Milvus 2.5+ requirement**
Nullable/default is a Milvus 2.5 feature (the PR links to the v2.5.x docs
and the E2E image is bumped to `v2.5.11`). A user on Milvus 2.4 setting
`enable_nullable_field=true` will hit `createCollection` failures. The en/zh
option rows match the code (name/default/type/description look good), but
please add a "Requires Milvus >= 2.5" note to both docs.
**4. Scope — this PR bundles several distinct concerns**
Per "one PR should solve one problem":
- `MilvusCatalog.tableExists` returning `false` when the database is missing
looks unrelated to the nullable feature (`createTable` already checks
`databaseExists` first), and it adds a `listDatabases` RPC to every
`tableExists` call. Consider splitting it into its own PR.
- The E2E image bump `2.4-20240711-...` -> `v2.5.11` is necessary for the
nullable test, but it changes the image for *all* existing Milvus E2E tests.
Please confirm CI is green across them.
- The new `FLOAT_VECTOR` case in `seatunnel-format-json`
`JsonToRowConverters` is a shared-format change only loosely related to "Milvus
nullable", and it's incomplete (see #5).
**5. `convertToFloatVector` is incomplete**
It only handles `FLOAT_VECTOR`; `BINARY_VECTOR` / `FLOAT16_VECTOR` /
`BFLOAT16_VECTOR` / `SPARSE_FLOAT_VECTOR` still fall through to
`UNSUPPORTED_DATA_TYPE`, which is inconsistent. Also `values[i] =
convertToFloat(jsonNode.get(i))` will throw a raw `NumberFormatException` on a
`NullNode` or non-numeric element without per-element context. Please either
cover all vector types or document the limitation, and add a unit test in
`seatunnel-format-json` (currently only E2E-covered).
--
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]
