nevzheng commented on code in PR #17138:
URL: https://github.com/apache/iceberg/pull/17138#discussion_r3555315607
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2561,7 +2565,22 @@ components:
- "is-nan"
- "not-nan"
+ ValueExpression:
+ description: >
+ A value expression per Iceberg Expressions spec, Appendix B: a literal,
Review 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.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2530,13 +2530,17 @@ components:
Expression:
oneOf:
+ - type: boolean
Review 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.
```txt
// deprecated.
...list deprecated
// Iceberg Expression Spec, Appendix B
... list current
```
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
+ - type: number
+ - type: integer
+ - type: boolean
+ - type: 'null'
+ - type: object
+ required:
+ - type
+ - value
+ properties:
+ type:
+ type: string
+ const: "literal"
+ value:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ Literals:
+ description: >
+ A set of literal values per Iceberg Expressions spec, Appendix B.
+ Two JSON forms: a bare array of literals, or a typed object with an
+ explicit data-type applied to all values.
+ oneOf:
+ - type: array
+ items:
+ $ref: '#/components/schemas/Literal'
+ - type: object
+ required:
+ - type
+ - values
+ properties:
+ type:
+ type: string
+ const: "literals"
+ values:
+ type: array
+ items:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ 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
+ name). The context in which an expression is used determines which
+ form is valid.
+ oneOf:
+ - type: object
+ required:
+ - type
+ - id
+ properties:
+ type:
+ type: string
+ const: "reference"
+ id:
+ type: integer
+ - type: object
+ required:
+ - type
+ - name
+ properties:
+ type:
+ type: string
+ const: "reference"
+ name:
+ type: string
+
+ Apply:
+ description: >
+ A function application per Iceberg Expressions spec, Appendix B.
+ Represents the result of calling a function on zero or more value
+ expressions or predicates.
+ type: object
+ required:
+ - type
+ - function
+ - arguments
+ properties:
+ type:
+ type: string
+ const: "apply"
+ function:
+ $ref: '#/components/schemas/FunctionReference'
+ arguments:
Review 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* ] }
```
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
+ - type: number
+ - type: integer
+ - type: boolean
+ - type: 'null'
+ - type: object
+ required:
+ - type
+ - value
+ properties:
+ type:
+ type: string
+ const: "literal"
+ value:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ Literals:
+ description: >
+ A set of literal values per Iceberg Expressions spec, Appendix B.
+ Two JSON forms: a bare array of literals, or a typed object with an
+ explicit data-type applied to all values.
+ oneOf:
+ - type: array
+ items:
+ $ref: '#/components/schemas/Literal'
+ - type: object
+ required:
+ - type
+ - values
+ properties:
+ type:
+ type: string
+ const: "literals"
+ values:
+ type: array
+ items:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ FieldReference:
Review 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.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
Review Comment:
I would recommend NOT referring to past version in current code. It seems
unnecessary.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
Review 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
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
+ - type: number
+ - type: integer
+ - type: boolean
+ - type: 'null'
+ - type: object
+ required:
+ - type
+ - value
+ properties:
+ type:
+ type: string
+ const: "literal"
+ value:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ Literals:
+ description: >
+ A set of literal values per Iceberg Expressions spec, Appendix B.
+ Two JSON forms: a bare array of literals, or a typed object with an
+ explicit data-type applied to all values.
+ oneOf:
+ - type: array
+ items:
+ $ref: '#/components/schemas/Literal'
+ - type: object
+ required:
Review Comment:
the object form of `Literals` has to have a "data type"
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
Review Comment:
I would say `A predicate operates on a set of literals`. what value you
return depends on the specific implementation.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
+ - type: number
+ - type: integer
+ - type: boolean
+ - type: 'null'
+ - type: object
+ required:
+ - type
+ - value
+ properties:
+ type:
+ type: string
+ const: "literal"
+ value:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ Literals:
+ description: >
+ A set of literal values per Iceberg Expressions spec, Appendix B.
+ Two JSON forms: a bare array of literals, or a typed object with an
Review Comment:
This seems non compliant. Here's my reading of the spec.
```txt
LITERAL:
VALUE (X)
| { "type": "literal", "value": VALUE } (Y)
| { "type": "literal", "value": VALUE, "data-type": DATA_TYPE } (Z)
LITERALS:
// A LIST of 0 or More `Literal`. This is recursive. And you would end
up with a list any permutation of (X), (Y), (Z) definitions
[ LITERAL* ]
// A structure capturing VALUEs of the same `DATA_TYPE`.
| { "type": "literals", "values": [ VALUE* ], "data-type": DATA_TYPE }
```
- Its not a set (non ordered, unique collection))
I would just let the recursive definition breathe.
```txt
`Literals is an array of `Literal` or, an object with an array of Value of
the same type, and a data type describing the type.
```
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
Review Comment:
can we pack the `VALUE` into a structure to make it look more like the spec.
I think this is probably ok.
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2606,7 +2630,194 @@ components:
child:
$ref: '#/components/schemas/Expression'
+ UnaryPredicate:
+ description: >
+ A predicate that tests a single value expression. Replaces the
deprecated
+ UnaryExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["is-null", "not-null", "is-nan", "not-nan"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+
+ ComparisonPredicate:
+ description: >
+ A predicate that compares two value expressions. Replaces the
deprecated
+ LiteralExpression that used 'term' and 'value'.
+ type: object
+ required:
+ - type
+ - left
+ - right
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq", "starts-with",
"not-starts-with"]
+ left:
+ $ref: '#/components/schemas/ValueExpression'
+ right:
+ $ref: '#/components/schemas/ValueExpression'
+
+ SetPredicate:
+ description: >
+ A predicate that tests whether a value expression is in a set of
literals.
+ Replaces the deprecated SetExpression that used 'term'.
+ type: object
+ required:
+ - type
+ - child
+ - values
+ properties:
+ type:
+ $ref: '#/components/schemas/ExpressionType'
+ enum: ["in", "not-in"]
+ child:
+ $ref: '#/components/schemas/ValueExpression'
+ values:
+ $ref: '#/components/schemas/Literals'
+
+ Literal:
+ description: >
+ A literal value expression per Iceberg Expressions spec, Appendix B.
+ Three JSON forms are accepted: a bare scalar value, a typed literal
+ object without a data-type, or a typed literal object with an
+ explicit data-type.
+ oneOf:
+ - type: string
+ - type: number
+ - type: integer
+ - type: boolean
+ - type: 'null'
+ - type: object
+ required:
+ - type
+ - value
+ properties:
+ type:
+ type: string
+ const: "literal"
+ value:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ Literals:
+ description: >
+ A set of literal values per Iceberg Expressions spec, Appendix B.
+ Two JSON forms: a bare array of literals, or a typed object with an
+ explicit data-type applied to all values.
+ oneOf:
+ - type: array
+ items:
+ $ref: '#/components/schemas/Literal'
+ - type: object
+ required:
+ - type
+ - values
+ properties:
+ type:
+ type: string
+ const: "literals"
+ values:
+ type: array
+ items:
+ $ref: '#/components/schemas/PrimitiveTypeValue'
+ data-type:
+ $ref: '#/components/schemas/PrimitiveType'
+
+ 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
Review Comment:
I would suggest
```txt
A reference to a field by id or name.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
