You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2026/07/23 09:00:30 UTC
[PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
singhpk234 opened a new pull request, #17343:
URL: https://github.com/apache/iceberg/pull/17343
PR Description
## Summary
Updates the Java `ExpressionParser` to serialize predicates in the new
form defined in [`format/expressions-spec.md`](https://github.com/apache/iceberg/blob/main/format/expressions-spec.md#appendix-b-json-serialization),
while continuing to accept the deprecated term-based form for backward
compatibility.
This is the Java-side counterpart to #17138, which updates the REST
OpenAPI spec to the new `Predicate` / `ValueExpression` model.
## Changes
### New model classes (`api` module)
- `FunctionReference` — value class for function references, supporting
the four spec forms: bare name, name array, `{identifier}`, and
`{catalog, identifier}`.
- `UnboundApply<T>` — implements `UnboundTerm<T>` to represent general
function applications. `bind()` delegates to `UnboundTransform` for
known Iceberg transforms (identity, year, month, day, hour, bucket,
truncate, void); other functions currently throw at bind time.
### Parser changes (`core` module)
- **Writer** now emits the new form only:
- Predicates use `child` / `left` / `right` instead of `term` / `value`
- References serialize as `{"type": "reference", "name": ...}` for
unbound and `{"type": "reference", "id": N}` for bound
- Transforms serialize as `apply` with the
arguments (e.g. `bucket[100](id)` → `{"type": "apply",
"function": "bucket", "arguments": [100, {"type": "reference",
"name": "id"}]}`)
- **Reader** detects the format from the payload and accepts both:
- If a predicate has a `term` field, the deprecated path parses it
(existing behavior)
- Otherwise the new path parses `child` / `left` / `right`,
references (`name` or deprecated `term`), and `apply` expressions
- Known Iceberg transforms parsed from `apply` are converted back to
`UnboundTransform` so downstream binding paths are unchanged
### Tests
- Updated existing round-trip and expected-JSON assertions in
`TestExpressionParser`, `TestPlanTableScanRequestParser`,
`TestFileScanTaskParser`, `TestPlanTableScanResponseParser`,
`TestFetchPlanningResultResponseParser`,
`TestFetchScanTasksResponseParser`,
`TestAllManifestsTableTaskParser`, and `TestFilesTableTaskParser` to
the new writer output.
- Added backward-compatibility reader tests for each deprecated
predicate form (`testReadDeprecatedUnaryPredicate`,
`testReadDeprecatedComparisonPredicate`,
`testReadDeprecatedSetPredicate`,
`testReadDeprecatedTransformPredicate`,
`testReadDeprecatedObjectReference`).
- Added new-format tests: `testNewFormatRoundTrip`,
`testApplySimpleFunctionName`, `testApplyWithCatalogFunction`,
`testNewFormatNamedReference`, `testNewFormatTypedLiteral`.
## Test plan
- [x] `./gradlew :iceberg-api:test`
- [x] `./gradlew :iceberg-core:test --tests "org.apache.iceberg.expressions.*"`
- [x] `./gradlew :iceberg-core:test --tests "org.apache.iceberg.rest.*"`
- [x] `./gradlew :iceberg-core:test --tests "org.apache.iceberg.metrics.*"`
- [x] `./gradlew :iceberg-core:test --tests "org.apache.iceberg.TestFileScanTaskParser"`
- [x] `./gradlew spotlessApply`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169911025
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -50,6 +59,30 @@ public class ExpressionParser {
private static final String CHILD = "child";
private static final String REFERENCE = "reference";
private static final String LITERAL = "literal";
+ private static final String LITERALS = "literals";
+ private static final String DATA_TYPE = "data-type";
+ private static final String APPLY = "apply";
+ private static final String FUNCTION = "function";
+ private static final String ARGUMENTS = "arguments";
+ private static final String NAME = "name";
+ private static final String ID = "id";
+ private static final String IDENTIFIER = "identifier";
+ private static final String CATALOG = "catalog";
+
+ private static final Pattern HAS_WIDTH = Pattern.compile("(\\w+)\\[(\\d+)]");
+
+ private static final String ICEBERG_FUNCTIONS = "iceberg_functions";
+ // the expressions spec defines partition transforms as functions, other than void
Review Comment:
This is true, but they must be in the `iceberg_functions` catalog. And I don't think that we need to return the `Transform` from the parser.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832404971
##########
api/src/main/java/org/apache/iceberg/expressions/FunctionReference.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.io.Serializable;
+import java.util.List;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/**
+ * Represents a function reference in an expression. A function reference identifies a function by
+ * name, optionally qualified with a catalog.
+ *
+ * <p>Use {@link Expressions#function(String)} and its overloads to create instances.
+ */
+public class FunctionReference implements Serializable {
+ private final String catalog;
+ private final List<String> identifier;
+
+ FunctionReference(String catalog, List<String> identifier) {
+ Preconditions.checkArgument(
+ identifier != null && !identifier.isEmpty(), "Function identifier cannot be null or empty");
+ Preconditions.checkArgument(
+ identifier.stream().noneMatch(part -> part == null || part.isEmpty()),
+ "Function identifier cannot contain empty parts: %s",
+ identifier);
+ this.catalog = catalog;
+ // not an immutable list so that Kryo can deserialize this class
+ this.identifier = Lists.newArrayList(identifier);
+ }
+
+ public String catalog() {
+ return catalog;
+ }
+
+ public List<String> identifier() {
+ return identifier;
+ }
+
+ /** Returns the last element of the identifier (the function name). */
+ public String name() {
+ return identifier.get(identifier.size() - 1);
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof FunctionReference)) {
Review Comment:
Style: control flow blocks should be separated from the following statement by an empty newline.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169835186
##########
api/src/test/java/org/apache/iceberg/expressions/TestFunctionReference.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.Arrays;
+import org.apache.iceberg.TestHelpers;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.jupiter.api.Test;
+
+public class TestFunctionReference {
+
+ @Test
+ public void name() {
+ assertThat(Expressions.function("bucket").name()).isEqualTo("bucket");
+ assertThat(Expressions.function(ImmutableList.of("ns", "sub", "func")).name())
+ .isEqualTo("func");
+ assertThat(Expressions.function("ns", "sub", "func").name()).isEqualTo("func");
+ }
+
+ @Test
+ public void identifierPartsAreNotParsed() {
+ FunctionReference ref = Expressions.function("ns.func");
+ assertThat(ref.identifier()).containsExactly("ns.func");
+ assertThat(ref.name()).isEqualTo("ns.func");
+ }
+
+ @Test
+ public void identifier() {
+ FunctionReference ref = Expressions.function("cat", ImmutableList.of("ns", "func"));
+ assertThat(ref.catalog()).isEqualTo("cat");
+ assertThat(ref.identifier()).containsExactly("ns", "func");
+ assertThat(Expressions.function("func").catalog()).isNull();
Review Comment:
This is a different test case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update Expressions to match Expressions spec [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4200375895
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -178,24 +215,26 @@ public <T> Void predicate(UnboundPredicate<T> pred) {
return generate(
() -> {
gen.writeStartObject();
-
gen.writeStringField(TYPE, operationType(pred.op()));
- gen.writeFieldName(TERM);
- term(pred.term());
- if (pred.literals() != null) {
- if (pred.op() == Expression.Operation.IN
- || pred.op() == Expression.Operation.NOT_IN) {
- gen.writeArrayFieldStart(VALUES);
+ if (pred.op() == Expression.Operation.IN || pred.op() == Expression.Operation.NOT_IN) {
Review Comment:
nothing major just trying to inline this in one block ... reverted it
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169798115
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ private final FunctionReference function;
+ private final List<Object> arguments;
+
+ UnboundApply(FunctionReference function, List<Object> arguments) {
+ Preconditions.checkArgument(function != null, "Invalid function: null");
+ this.function = function;
+ // not an immutable list so that Kryo can deserialize this class
Review Comment:
We typically use arrays to make things Kryo serializable
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832410031
##########
api/src/main/java/org/apache/iceberg/expressions/FunctionReference.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.io.Serializable;
+import java.util.List;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/**
+ * Represents a function reference in an expression. A function reference identifies a function by
+ * name, optionally qualified with a catalog.
+ *
+ * <p>Use {@link Expressions#function(String)} and its overloads to create instances.
+ */
+public class FunctionReference implements Serializable {
+ private final String catalog;
+ private final List<String> identifier;
+
+ FunctionReference(String catalog, List<String> identifier) {
+ Preconditions.checkArgument(
+ identifier != null && !identifier.isEmpty(), "Function identifier cannot be null or empty");
+ Preconditions.checkArgument(
+ identifier.stream().noneMatch(part -> part == null || part.isEmpty()),
+ "Function identifier cannot contain empty parts: %s",
+ identifier);
+ this.catalog = catalog;
+ // not an immutable list so that Kryo can deserialize this class
+ this.identifier = Lists.newArrayList(identifier);
+ }
+
+ public String catalog() {
+ return catalog;
+ }
+
+ public List<String> identifier() {
+ return identifier;
+ }
+
+ /** Returns the last element of the identifier (the function name). */
+ public String name() {
+ return identifier.get(identifier.size() - 1);
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review Comment:
Why are `equals` and `hashCode` needed? These could be misleading because two references may identify the same function.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832433541
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.transforms.Transforms;
+import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions. Known Iceberg partition transforms can be converted to {@link
+ * UnboundTransform} via {@link #asTransform()}.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ // bucket and truncate take the transform parameter as their first argument, so they cannot be
+ // resolved from a name alone
+ private static final Set<String> PARAMETERIZED_TRANSFORMS = ImmutableSet.of("bucket", "truncate");
+
+ // the expressions spec defines partition transforms as functions, other than void
+ private static final String VOID = "void";
+
+ private static final String ICEBERG_FUNCTIONS_CATALOG = "iceberg_functions";
+
+ private final FunctionReference function;
+ private final List<Object> arguments;
Review Comment:
I doubt that `Object` is correct here. These must be either Expression (a predicate) or Term (a value expression).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169850532
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -152,16 +185,20 @@ public <T> Void predicate(BoundPredicate<T> pred) {
return generate(
() -> {
gen.writeStartObject();
-
Review Comment:
Let's leave whitespace.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832398921
##########
api/src/main/java/org/apache/iceberg/expressions/FunctionReference.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.io.Serializable;
+import java.util.List;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/**
+ * Represents a function reference in an expression. A function reference identifies a function by
+ * name, optionally qualified with a catalog.
+ *
+ * <p>Use {@link Expressions#function(String)} and its overloads to create instances.
+ */
+public class FunctionReference implements Serializable {
+ private final String catalog;
+ private final List<String> identifier;
+
+ FunctionReference(String catalog, List<String> identifier) {
+ Preconditions.checkArgument(
+ identifier != null && !identifier.isEmpty(), "Function identifier cannot be null or empty");
+ Preconditions.checkArgument(
+ identifier.stream().noneMatch(part -> part == null || part.isEmpty()),
+ "Function identifier cannot contain empty parts: %s",
Review Comment:
`Invalid function identifier (empty or null part): %s`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169822615
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ private final FunctionReference function;
+ private final List<Object> arguments;
+
+ UnboundApply(FunctionReference function, List<Object> arguments) {
+ Preconditions.checkArgument(function != null, "Invalid function: null");
+ this.function = function;
+ // not an immutable list so that Kryo can deserialize this class
+ this.arguments =
+ arguments == null
+ ? Lists.newArrayList()
+ : Lists.newArrayList(Lists.transform(arguments, UnboundApply::toArgument));
+ }
+
+ /**
+ * Converts an argument to the type used to represent it, or throws if it cannot be an argument.
+ *
+ * <p>An argument is either a value expression or a predicate. Value expressions are {@link Term}
+ * (a reference or a nested apply) or a constant, which is converted to a {@link Literal}.
+ * Predicates are {@link Expression}.
+ */
+ private static Object toArgument(Object argument) {
+ Preconditions.checkArgument(argument != null, "Invalid function argument: null");
+ if (argument instanceof Term || argument instanceof Expression) {
+ return argument;
+ }
+
+ return Literals.from(argument);
+ }
+
+ public FunctionReference function() {
+ return function;
+ }
+
+ /**
+ * Returns the arguments passed to the function.
+ *
+ * <p>Each argument is a {@link Term} (a value expression), an {@link Expression} (a predicate),
+ * or a {@link Literal} (a constant value expression). Java has no union type, so the arguments
+ * are typed as {@link Object} and validated when this expression is created.
+ */
+ public List<Object> arguments() {
+ return arguments;
+ }
+
+ @Override
+ public NamedReference<T> ref() {
+ // a function may be called with any number of references, so there is no single reference that
+ // this term produces values from
+ throw new UnsupportedOperationException("Cannot determine reference for function: " + function);
+ }
+
+ @Override
+ public BoundTerm<T> bind(Types.StructType struct, boolean caseSensitive) {
+ // binding requires a function definition to determine the result type, and function definitions
+ // are not available in this API
+ throw new UnsupportedOperationException("Cannot bind function: " + function);
Review Comment:
This should recursively call `bind` on each argument and return a `BoundApply` from those bound arguments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832422716
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.transforms.Transforms;
+import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions. Known Iceberg partition transforms can be converted to {@link
+ * UnboundTransform} via {@link #asTransform()}.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ // bucket and truncate take the transform parameter as their first argument, so they cannot be
+ // resolved from a name alone
+ private static final Set<String> PARAMETERIZED_TRANSFORMS = ImmutableSet.of("bucket", "truncate");
Review Comment:
Why is this here? I don't see why this class would know anything about specific functions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3753100017
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -74,10 +90,6 @@ private JsonGeneratorVisitor(JsonGenerator gen) {
this.gen = gen;
}
- /**
Review Comment:
Not sure this needed to be removed?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169889288
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -178,24 +215,26 @@ public <T> Void predicate(UnboundPredicate<T> pred) {
return generate(
() -> {
gen.writeStartObject();
-
gen.writeStringField(TYPE, operationType(pred.op()));
- gen.writeFieldName(TERM);
- term(pred.term());
- if (pred.literals() != null) {
- if (pred.op() == Expression.Operation.IN
- || pred.op() == Expression.Operation.NOT_IN) {
- gen.writeArrayFieldStart(VALUES);
+ if (pred.op() == Expression.Operation.IN || pred.op() == Expression.Operation.NOT_IN) {
Review Comment:
What is the rationale for changing the structure? I think it would still work just fine, just with the term/child/left added at the same time. That would be a smaller change.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169894639
##########
core/src/main/java/org/apache/iceberg/expressions/ExpressionParser.java:
##########
@@ -226,37 +265,99 @@ private void unboundLiteral(Object object) throws IOException {
BigDecimal decimal = (BigDecimal) object;
SingleValueParser.toJson(
Types.DecimalType.of(decimal.precision(), decimal.scale()), decimal, gen);
+ } else {
+ throw new UnsupportedOperationException(
+ "Cannot write literal of unsupported type: " + object.getClass().getName());
}
}
private String operationType(Expression.Operation op) {
return op.toString().replace('_', '-').toLowerCase(Locale.ROOT);
}
- private void term(Term term) throws IOException {
- if (term instanceof UnboundTransform) {
+ private void writeExpr(Term term) throws IOException {
+ if (term instanceof UnboundApply) {
+ writeApply((UnboundApply<?>) term);
+ } else if (term instanceof UnboundTransform) {
Review Comment:
Shouldn't `UnboundTransform` be a subclass of `UnboundApply` so you don't need to handle it separately here?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832414335
##########
api/src/main/java/org/apache/iceberg/expressions/FunctionReference.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.io.Serializable;
+import java.util.List;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/**
+ * Represents a function reference in an expression. A function reference identifies a function by
+ * name, optionally qualified with a catalog.
+ *
+ * <p>Use {@link Expressions#function(String)} and its overloads to create instances.
+ */
+public class FunctionReference implements Serializable {
+ private final String catalog;
+ private final List<String> identifier;
+
+ FunctionReference(String catalog, List<String> identifier) {
+ Preconditions.checkArgument(
+ identifier != null && !identifier.isEmpty(), "Function identifier cannot be null or empty");
+ Preconditions.checkArgument(
+ identifier.stream().noneMatch(part -> part == null || part.isEmpty()),
+ "Function identifier cannot contain empty parts: %s",
+ identifier);
+ this.catalog = catalog;
+ // not an immutable list so that Kryo can deserialize this class
+ this.identifier = Lists.newArrayList(identifier);
+ }
+
+ public String catalog() {
+ return catalog;
+ }
+
+ public List<String> identifier() {
+ return identifier;
+ }
+
+ /** Returns the last element of the identifier (the function name). */
Review Comment:
Is this relevant information or is it leaking implementation details?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832389322
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -317,6 +318,49 @@ public static <T> UnboundTerm<T> transform(String name, Transform<?, T> transfor
return new UnboundTransform<>(ref(name), transform);
}
+ /**
+ * Create a reference to a function by name, in the catalog of the referencing object.
Review Comment:
I'm assuming that this is not going to parse the name in any way? If so, I'd like to see that noted here. I also think it is probably better to define this as `String...` so that you can capture multiple parts. Then the semantics are clear that you can pass a single name, but it won't be parsed (because it's easy to pass additional parts).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update Expressions to match Expressions spec [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4189672014
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -317,6 +319,55 @@ public static <T> UnboundTerm<T> transform(String name, Transform<?, T> transfor
return new UnboundTransform<>(ref(name), transform);
}
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * <p>Identifier parts are used as given and are never parsed or split. A part that contains dots
+ * is a single part, not a namespace followed by a function name.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String... identifier) {
+ return new FunctionReference(null, Arrays.asList(identifier));
+ }
+
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(List<String> identifier) {
+ return new FunctionReference(null, identifier);
+ }
+
+ /**
+ * Create a reference to a function in a specific catalog.
+ *
+ * @param catalog a catalog name
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String catalog, List<String> identifier) {
+ return new FunctionReference(catalog, identifier);
+ }
+
+ /**
+ * Create an expression that applies a function to zero or more arguments.
+ *
+ * <p>Each argument must be a value expression ({@link Term}), a predicate ({@link Expression}),
+ * or a constant. Constants are converted to {@link Literal}.
+ *
+ * @param function a function reference
+ * @param arguments value expressions, predicates, or constants passed to the function
+ * @param <T> the Java type of this term
+ * @return an unbound apply expression
+ */
+ public static <T> UnboundApply<T> apply(FunctionReference function, List<Object> arguments) {
Review Comment:
> This helper may need to be omitted until we have the type hierarchy built out with a type for ValueExpression + Predicate, which I think is called FUNC_ARG in the spec (maybe FunctionArg here?).
precisely, hence i couldn't find some thing better than Object, should i make this package private (so its not a public api ? ) and meanwhile when we have the hierarchy changes we could remove this ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832400460
##########
api/src/main/java/org/apache/iceberg/expressions/FunctionReference.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.io.Serializable;
+import java.util.List;
+import java.util.Objects;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+
+/**
+ * Represents a function reference in an expression. A function reference identifies a function by
+ * name, optionally qualified with a catalog.
+ *
+ * <p>Use {@link Expressions#function(String)} and its overloads to create instances.
+ */
+public class FunctionReference implements Serializable {
+ private final String catalog;
+ private final List<String> identifier;
+
+ FunctionReference(String catalog, List<String> identifier) {
+ Preconditions.checkArgument(
+ identifier != null && !identifier.isEmpty(), "Function identifier cannot be null or empty");
Review Comment:
`Invalid function identifier: %s`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r3832436051
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.transforms.Transforms;
+import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions. Known Iceberg partition transforms can be converted to {@link
+ * UnboundTransform} via {@link #asTransform()}.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ // bucket and truncate take the transform parameter as their first argument, so they cannot be
+ // resolved from a name alone
+ private static final Set<String> PARAMETERIZED_TRANSFORMS = ImmutableSet.of("bucket", "truncate");
+
+ // the expressions spec defines partition transforms as functions, other than void
+ private static final String VOID = "void";
+
+ private static final String ICEBERG_FUNCTIONS_CATALOG = "iceberg_functions";
+
+ private final FunctionReference function;
+ private final List<Object> arguments;
+
+ UnboundApply(FunctionReference function, List<Object> arguments) {
+ Preconditions.checkArgument(function != null, "Function reference cannot be null");
Review Comment:
`Invalid function: null`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update Expressions to match Expressions spec [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4200360004
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -317,6 +319,55 @@ public static <T> UnboundTerm<T> transform(String name, Transform<?, T> transfor
return new UnboundTransform<>(ref(name), transform);
}
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * <p>Identifier parts are used as given and are never parsed or split. A part that contains dots
+ * is a single part, not a namespace followed by a function name.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String... identifier) {
+ return new FunctionReference(null, Arrays.asList(identifier));
+ }
+
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(List<String> identifier) {
+ return new FunctionReference(null, identifier);
+ }
+
+ /**
+ * Create a reference to a function in a specific catalog.
+ *
+ * @param catalog a catalog name
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String catalog, List<String> identifier) {
+ return new FunctionReference(catalog, identifier);
+ }
+
+ /**
+ * Create an expression that applies a function to zero or more arguments.
+ *
+ * <p>Each argument must be a value expression ({@link Term}), a predicate ({@link Expression}),
+ * or a constant. Constants are converted to {@link Literal}.
+ *
+ * @param function a function reference
+ * @param arguments value expressions, predicates, or constants passed to the function
+ * @param <T> the Java type of this term
+ * @return an unbound apply expression
+ */
+ public static <T> UnboundApply<T> apply(FunctionReference function, List<Object> arguments) {
Review Comment:
> maybe FunctionArg here
Function Arg would be nice abstraction !
will let Dan chime in too, for now for the completeness of impl i just marked them as private, this can come as follow up as well and i am fine to remove this completely as well
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169790865
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.transforms.Transforms;
+import org.apache.iceberg.transforms.UnknownTransform;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions. Known Iceberg partition transforms can be converted to {@link
+ * UnboundTransform} via {@link #asTransform()}.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
+ // bucket and truncate take the transform parameter as their first argument, so they cannot be
+ // resolved from a name alone
+ private static final Set<String> PARAMETERIZED_TRANSFORMS = ImmutableSet.of("bucket", "truncate");
+
+ // the expressions spec defines partition transforms as functions, other than void
+ private static final String VOID = "void";
+
+ private static final String ICEBERG_FUNCTIONS_CATALOG = "iceberg_functions";
+
+ private final FunctionReference function;
+ private final List<Object> arguments;
Review Comment:
😄
I just made almost the exact same comment above.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169781241
##########
api/src/main/java/org/apache/iceberg/expressions/Expressions.java:
##########
@@ -317,6 +319,55 @@ public static <T> UnboundTerm<T> transform(String name, Transform<?, T> transfor
return new UnboundTransform<>(ref(name), transform);
}
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * <p>Identifier parts are used as given and are never parsed or split. A part that contains dots
+ * is a single part, not a namespace followed by a function name.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String... identifier) {
+ return new FunctionReference(null, Arrays.asList(identifier));
+ }
+
+ /**
+ * Create a reference to a function by identifier, in the catalog of the referencing object.
+ *
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(List<String> identifier) {
+ return new FunctionReference(null, identifier);
+ }
+
+ /**
+ * Create a reference to a function in a specific catalog.
+ *
+ * @param catalog a catalog name
+ * @param identifier namespace names followed by the function name
+ * @return a function reference
+ */
+ public static FunctionReference function(String catalog, List<String> identifier) {
+ return new FunctionReference(catalog, identifier);
+ }
+
+ /**
+ * Create an expression that applies a function to zero or more arguments.
+ *
+ * <p>Each argument must be a value expression ({@link Term}), a predicate ({@link Expression}),
+ * or a constant. Constants are converted to {@link Literal}.
+ *
+ * @param function a function reference
+ * @param arguments value expressions, predicates, or constants passed to the function
+ * @param <T> the Java type of this term
+ * @return an unbound apply expression
+ */
+ public static <T> UnboundApply<T> apply(FunctionReference function, List<Object> arguments) {
Review Comment:
The other methods look good, but I think `List<Object>` is too broad here. This helper may need to be omitted until we have the type hierarchy built out with a type for `ValueExpression + Predicate`, which I think is called `FUNC_ARG` in the spec (maybe `FunctionArg` here?).
I think @danielcweeks has been working on the hierarchy changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Update ExpressionParser to match new expressions spec [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #17343:
URL: https://github.com/apache/iceberg/pull/17343#discussion_r4169813916
##########
api/src/main/java/org/apache/iceberg/expressions/UnboundApply.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.expressions;
+
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.types.Types;
+
+/**
+ * Represents a function application expression. This is the general form for invoking functions on
+ * value expressions.
+ *
+ * @param <T> the Java type of values produced by this expression
+ */
+public class UnboundApply<T> implements UnboundTerm<T> {
Review Comment:
I think that this should be a `ValueExpression` type that it not a `Term`. `Term` is used for expressions that return a value based on a reference, but `ValueExpression` will be more broad. It will include literals (not `Term`) and these `Apply` expressions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org