OpenAPI: Update REST spec expressions to match new expressions spec#17138
OpenAPI: Update REST spec expressions to match new expressions spec#17138singhpk234 wants to merge 8 commits into
Conversation
Adds UnaryPredicate, ComparisonPredicate, and SetPredicate schemas that use 'child' and 'left'/'right' value expression references, matching the JSON serialization defined in Appendix B of the Iceberg Expressions spec (format/expressions-spec.md). The existing UnaryExpression, LiteralExpression, and SetExpression schemas (which use 'term'/'value'/'values') are marked deprecated but retained in Expression.oneOf for backward compatibility with older clients per the spec's "Backward compatibility" section. A ValueExpression placeholder schema is added and referenced by the new predicates; concrete LITERAL/REFERENCE/APPLY forms will be added in a follow-up change.
Extends Expression.oneOf with a bare `boolean` alternative, matching the
spec-conformant PREDICATE production in the Iceberg Expressions spec
(format/expressions-spec.md, Appendix B) which defines `PREDICATE: true
| false | { ... }`.
The pre-existing TrueExpression and FalseExpression object-form schemas
(`{"type": "true"}` / `{"type": "false"}`) are marked deprecated and
retained in Expression.oneOf for backward compatibility with older
clients that send the object form.
…endix B Replaces the ValueExpression placeholder with concrete oneOf schemas for LITERAL, REFERENCE, and APPLY forms, matching Appendix B of the Iceberg Expressions spec. Also adds the deprecated reference form for backward compatibility with older REST predicates. Schemas added: - Literal (bare scalar, typed literal object with/without data-type) - Literals (bare array or typed object; used by SetPredicate.values) - FieldReference (bound by id or unbound by name) - Apply (function application over ValueExpression or Expression args) - FunctionReference (four forms: name, name list, identifier object, catalog + identifier object) - DeprecatedReference (backward-compat form wired into Term's oneOf) SetPredicate.values now references Literals rather than an array of ValueExpression to match Appendix B's LITERALS grammar.
Apply spec-mandated fixes surfaced by adversarial review against
format/expressions-spec.md Appendix B:
- Literal.oneOf: remove the type:integer branch — every integer already
matches type:number, so the duplicate broke strict oneOf validators.
- Apply.arguments.items: switch oneOf to anyOf. Spec line 265 defines
FUNC_ARG as EXPR | PREDICATE (semantic "or"), and a bare boolean
legitimately matches both branches; oneOf's "exactly one" rejects it.
- FieldReference and DeprecatedReference: add additionalProperties:false
so the shared type:"reference" discriminator resolves to exactly one
branch. Without it, payloads mixing id/name/term validate ambiguously.
- Literals typed-object form: mark data-type required per spec line 258
grammar (it appears with no optional marker).
- FunctionReference: add additionalProperties:false to the {identifier}
form so it is disjoint from the {catalog, identifier} form (spec lines
267-269 enumerate them as distinct productions). Add minItems:1 to
every identifier array per spec line 82: "At least one part, the
function name, is required."
nevzheng
left a comment
There was a problem hiding this comment.
Hi Prashant!
Good work!
(a) I think there could be some spec compatibility issues that I noted. In some definitions, we don't follow the exact rules specified, which may cause issues. I think it depends on how strictly we want to follow the spec. As written, or interpreted.
(b) I think we could make the descriptions more concise.
- I highlighted that indicating "deprecated vs current" could just go one way instead of both.
- We have a lot of references to Iceberg Expression Spec Appendix B. I'm wondering if there could be a more concise way to do it?
- I think we could adopt a description wording strategy? like "X is either/or/just [A] [B] [C]. And maybe we could allow for text based recursive definition?
(c) I think its worth considering if we should define FieldReference as a tagged enum of FieldReferenceById or FieldReferenceByName.
I will defer to the group consensus.
Thanks,
Nevin
|
|
||
| ValueExpression: | ||
| description: > | ||
| A value expression per Iceberg Expressions spec, Appendix B: a literal, |
There was a problem hiding this comment.
How about A value is a literal, field reference, or function application. See Iceberg Expressions spec, Appendix B. I think the result type part doesn't add anything.
|
|
||
| Expression: | ||
| oneOf: | ||
| - type: boolean |
There was a problem hiding this comment.
What if we did something like this. I'm thinking we wouldn't have to mention "Iceberg Expression Spec in description this way" It seems a bit verbose.
// deprecated.
...list deprecated
// Iceberg Expression Spec, Appendix B
... list current| child: | ||
| $ref: '#/components/schemas/Expression' | ||
|
|
||
| UnaryPredicate: |
There was a problem hiding this comment.
I'm not sure this follows the spec. For example, we don't have Predicate= true | false | UnaryOp | CmpOp |....
We flatten by a level, and define UnaryPredidicate, SetPredicate, ComparisonPredicate...
Is that acceptable for spec compliance? I would defer to Ryan.
https://github.com/apache/iceberg/blob/main/format/expressions-spec.md
|
|
||
| UnaryPredicate: | ||
| description: > | ||
| A predicate that tests a single value expression. Replaces the deprecated |
There was a problem hiding this comment.
I would recommend NOT referring to past version in current code. It seems unnecessary.
|
|
||
| SetPredicate: | ||
| description: > | ||
| A predicate that tests whether a value expression is in a set of literals. |
There was a problem hiding this comment.
I would prefer a more neutral wording. A predicate operates on a set of literals.
The current wording seems to imply or favor the in variant,
| items: | ||
| $ref: '#/components/schemas/Literal' | ||
| - type: object | ||
| required: |
There was a problem hiding this comment.
the object form of Literals has to have a "data type"
| object without a data-type, or a typed literal object with an | ||
| explicit data-type. | ||
| oneOf: | ||
| - type: string |
There was a problem hiding this comment.
can we pack the VALUE into a structure to make it look more like the spec. I think this is probably ok.
| FieldReference: | ||
| description: > | ||
| A reference to a field per Iceberg Expressions spec, Appendix B. | ||
| Either a bound reference (by field ID) or an unbound reference (by |
There was a problem hiding this comment.
I would suggest
A reference to a field by id or name.| data-type: | ||
| $ref: '#/components/schemas/PrimitiveType' | ||
|
|
||
| FieldReference: |
There was a problem hiding this comment.
Should we have two types? We're allowed to define our own. FieldReferenceByName, FieldReferenceById, `FieldReference (FieldReferenceByName| FieldReferenceById). This allows expressions to specify which variant they accept.
FieldReference:
description: >
A reference to a field, by field ID or by name.
oneOf:
- $ref: '#/components/schemas/FieldReferenceById'
- $ref: '#/components/schemas/FieldReferenceByName'
FieldReferenceById:
description: A reference to a field by field ID (bound).
type: object
required:
- type
- id
properties:
type:
type: string
const: "reference"
id:
type: integer
FieldReferenceByName:
description: A reference to a field by name (unbound).
type: object
required:
- type
- name
properties:
type:
type: string
const: "reference"
name:
type: string
| const: "apply" | ||
| function: | ||
| $ref: '#/components/schemas/FunctionReference' | ||
| arguments: |
There was a problem hiding this comment.
I think we could be missing variants.
Arguments is a list of function arg. And Function arg is one of 4 things. and we only have 2 here.
APPLY: { "type": "apply", "function": FUNC_REF, "arguments": [ FUNC_ARG* ] }
FUNC_ARG: EXPR | PREDICATE
FUNC_REF: NAME
| [ NAME* ]
| { "identifier": [ NAME* ] }
| { "catalog": NAME, "identifier": [ NAME* ] }
Additional spec-fidelity fixes surfaced by iterated compliance/divergence
review against format/expressions-spec.md Appendix B:
- Literal typed-object, Literals typed-object, Apply, and FunctionReference
{catalog, identifier} all get additionalProperties:false so their JSON
shapes match the closed grammar productions in spec lines 255-269.
- LiteralExpression.value now refs Literal (was PrimitiveTypeValue) — spec
line 304 explicitly says the deprecated CMP_OP form takes LITERAL.
- SetExpression.values now refs Literals (was inline PrimitiveTypeValue
array) — spec line 305 says LITERALS, which includes bare-array and
typed-object forms.
- TransformTerm.term now refs Term (was Reference) — spec line 310 has
term:TERM, which is recursive and admits nested TRANSFORM and
DEPRECATED_REF, not only bare NAMEs.
- Literal bare oneOf drops type:'null' — Iceberg VALUE grammar (spec 254,
274) is a single scalar value; the typed form's PrimitiveTypeValue has
no null branch either, so the null form was asymmetric and dead-weight.
- FunctionReference NAME strings (bare-string form and identifier array
items across both object forms) get minLength:1 — spec line 82 requires
the function name to be a real identifier, not empty.
|
Following up on an earlier thought. If possible, I'd recommend following the grammar as literally as OpenAPI allows. I hope this will be clearer, and save us from misinterpretations, and back-and-forth time. |
No description provided.