mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 10:15:43 +00:00
590a14fdf7
issue: #50882 ## Problem `NULL` is not a grammar token in the plan parser, so the lexer treats a bare `NULL`/`null` as an ordinary identifier. When it appears where a value is expected — e.g. inside an `in [...]` value list: ``` id in [6560, NULL, 6722, -7856, -6757] ``` `VisitIdentifier` runs a field lookup on `NULL`: - **without a dynamic field** it fails with the misleading `field NULL not exist`, which misleads users into thinking they must add a column named `NULL` to their schema; - **with a dynamic field** it is silently mistaken for a JSON key and accepted. This affects both `delete()` and `query()` (they share the same `ParseExpr` path). ## Fix Treat bare `null`/`NULL` (case-insensitive) as a reserved word in `VisitIdentifier` and reject it up-front with an actionable message, regardless of whether a dynamic field exists: ``` NULL literal is not supported in expressions; use '<field> is null' or '<field> is not null' instead ``` This implements option 1 (robust rejection) from the issue. `<field> is null` / `is not null` are unaffected (they parse via dedicated tokens, not `VisitIdentifier`). **The guard is schema-aware for backward compatibility** (review feedback): `null` only becomes a create-time keyword in this PR, so a legacy collection may own a field literally named `null`, and the bare identifier is the only syntax that can reference a top-level scalar field. The rejection is therefore gated on a strict `GetFieldFromName` lookup — a real declared field named `null` resolves exactly as before this PR (including `null is null`, which now parses as a valid predicate on that field), while the dynamic-field fallback (the source of the original misparse) still rejects. A JSON **sub-key** literally named `null` additionally remains reachable via quoting, e.g. `field["null"]` / `$meta["null"]`, whose base identifier is the field name, not `null`. ## Test Added `TestExpr_NullLiteral` covering rejection of `NULL` inside `in [...]`, as a comparison operand, and as a left operand, plus confirmation that `is null` / `is not null` still work. Added `TestExpr_NullLiteral_LegacyNullField` locking the legacy path: a scalar field named `null` stays queryable via the bare identifier, a JSON field named `null` stays reachable as `null["x"]`, and any casing that doesn't name a real field keeps the reserved-word rejection. Full `planparserv2` package tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Note: field-name validation tightened Alongside the expression-level rejection, `validateFieldName` (proxy) and `validateAddedStructFieldName` (rootcoord) now go through `common.IsFieldNameKeyword`, which rejects **any casing** of `null` (`null`/`NULL`/`nUlL`/…) as a field name — consistent with how a bare `NULL` literal is rejected in expressions. Previously `FieldNameKeywords` had no `null` entry, so a field literally named `null` could be created. This only affects **new** field creation (validation runs at create time); existing collections/fields are unaffected — and thanks to the schema-aware guard above, a legacy field literally named `null` also stays queryable. --------- Signed-off-by: xiaofanluan <xf@hjjaq.com> Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com> Co-authored-by: xiaofanluan <xf@hjjaq.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
14 lines
275 B
Go
14 lines
275 B
Go
package common
|
|
|
|
import (
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
)
|
|
|
|
func TestIsFieldNameKeywordRejectsNullCaseInsensitive(t *testing.T) {
|
|
for _, name := range []string{"null", "Null", "NULL", "nUlL", "NuLL"} {
|
|
assert.True(t, IsFieldNameKeyword(name), name)
|
|
}
|
|
}
|