You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "Mihir-Rabari (via GitHub)" <gi...@apache.org> on 2026/10/17 12:58:36 UTC
[PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Mihir-Rabari opened a new pull request, #3370:
URL: https://github.com/apache/hugegraph/pull/3370
The Cypher endpoint rejects the standard `date()` constructor with
`Unknown function 'date'` because the cypher-for-gremlin translator ships a
fixed function list that contains no temporal functions. That list lives
inside the translator dependency, so there is no way to register new
functions from HugeGraph; instead I fold the temporal constructors on the
HugeGraph side. Right before translation, `date()`, `datetime()` and
`localdatetime()` calls with a foldable argument (no argument, a string
literal, or a parameter bound to a string) are rewritten into a synthetic
bound parameter carrying a `java.util.Date`, the value HugeGraph already
uses for DATE properties.
Calls that cannot be folded (`date(n.x)`, `date('tomorrow')`, map-argument
forms) and the remaining temporal functions (`time()`, `localtime()`,
`duration()`) are left untouched and keep failing with the translator's
original error, so behavior only changes for queries that used to be
rejected outright.
Notes on the implementation:
- The rewrite runs on a copy of the parsed statement and the copy is swapped
in only when the whole fold succeeds, so a fold failure degrades to
today's error instead of a half-rewritten query.
- The synthetic parameter names are prefixed with two spaces, the same
reserved-name trick the translator uses for its generated bindings, so
they cannot collide with names a query can express.
- The bare `RETURN date()` fixture in CypherErrorTest now translates (it
folds to the current date), so I replaced it with `RETURN duration('P1Y')`
to keep the unknown-function error path covered.
- Unit tests cover literal, parameter, bare, nested and CREATE-side folds
plus the non-foldable fallbacks; CypherApiTest gains end-to-end cases for
matching a DATE property and for a date value meeting a TEXT property.
Fixes #3318
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#issuecomment-6239984684
## [Codecov](https://app.codecov.io/gh/apache/hugegraph/pull/3370?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `75.15528%` with `40 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 42.46%. Comparing base ([`92af2f8`](https://app.codecov.io/gh/apache/hugegraph/commit/92af2f89c320c3e6fa7c10765a061f3edadea319?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fe0c1bb`](https://app.codecov.io/gh/apache/hugegraph/commit/fe0c1bb5d1ceb9349830c7be90b80a16d368a4c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 29 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/hugegraph/pull/3370?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../hugegraph/opencypher/CypherTemporalFunctions.java](https://app.codecov.io/gh/apache/hugegraph/pull/3370?src=pr&el=tree&filepath=hugegraph-server%2Fhugegraph-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhugegraph%2Fopencypher%2FCypherTemporalFunctions.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvb3BlbmN5cGhlci9DeXBoZXJUZW1wb3JhbEZ1bmN0aW9ucy5qYXZh) | 75.00% | [22 Missing and 18 partials :warning: ](https://app.codecov.io/gh/apache/hugegraph/pull/3370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3370 +/- ##
=============================================
- Coverage 54.31% 42.46% -11.85%
+ Complexity 2250 2 -2248
=============================================
Files 770 508 -262
Lines 67952 49043 -18909
Branches 9213 7116 -2097
=============================================
- Hits 36905 20827 -16078
+ Misses 27470 25796 -1674
+ Partials 3577 2420 -1157
```
| [Flag](https://app.codecov.io/gh/apache/hugegraph/pull/3370/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [server_rocksdb](https://app.codecov.io/gh/apache/hugegraph/pull/3370/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `42.46% <75.15%> (?)` | |
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hugegraph/pull/3370?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(server): fold cypher temp-construct into bound date values [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin merged PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4304830530
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,334 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.text.ParsePosition;
+import java.text.SimpleDateFormat;
+import java.time.LocalDate;
+import java.time.ZoneId;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ private static final String[] DATE_FORMATS = {"yyyy-MM-dd"};
+ private static final String[] DATETIME_FORMATS = {"yyyy-MM-dd'T'HH:mm:ss",
+ "yyyy-MM-dd'T'HH:mm:ss.SSS"};
+
+ // Two leading spaces: parameter names coming from a Cypher query can
+ // never collide with this prefix, like the translator's own reserved names.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ } catch (Exception | LinkageError e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ private static final class TemporalFold {
+
+ private final Map<String, Object> parameters;
+ private final Map<String, Object> bindings = new LinkedHashMap<>();
+ private int counter = 0;
+
+ private TemporalFold(Map<String, Object> parameters) {
+ this.parameters = parameters;
+ }
+
+ /**
+ * Returns a copy of the node with temporal constructors folded in, or
+ * the same instance when the subtree does not change. Subtrees that
+ * cannot be rebuilt are shared with the original statement, so the
+ * copy stays minimal.
+ */
+ private Object copy(Object node) throws Exception {
+ if (node == null || node instanceof String || node instanceof Number ||
+ node instanceof Boolean || node instanceof Character) {
+ return node;
+ }
+ if (node instanceof FunctionInvocation) {
+ Object folded = foldCall((FunctionInvocation) node);
+ if (folded != null) {
+ return folded;
+ }
+ }
+ if (node instanceof Option) {
+ Option<?> option = (Option<?>) node;
+ if (!option.isEmpty()) {
+ Object value = option.get();
+ Object copied = copy(value);
+ if (copied != value) {
+ return new Some<>(copied);
+ }
+ }
+ return node;
+ }
+ if (node instanceof scala.collection.IndexedSeq) {
+ return copyIndexedSeq((scala.collection.IndexedSeq<?>) node);
+ }
+ if (node instanceof Seq) {
+ return copySeq((Seq<?>) node);
+ }
+ if (node instanceof scala.collection.immutable.Set) {
+ return copySet((scala.collection.immutable.Set<?>) node);
+ }
+ if (node instanceof Tuple2) {
+ Tuple2<?, ?> tuple = (Tuple2<?, ?>) node;
+ Object first = copy(tuple._1());
+ Object second = copy(tuple._2());
+ return first == tuple._1() && second == tuple._2() ? node : new Tuple2<>(first, second);
+ }
+ if (node instanceof Product) {
+ return rebuild((Product) node);
+ }
+ return node;
+ }
+
+ private Object foldCall(FunctionInvocation call) {
+ String name = call.name();
+ if (!DATE.equals(name) && !DATETIME.equals(name) && !LOCALDATETIME.equals(name)) {
+ return null;
+ }
+ if (call.distinct() || !call.namespace().parts().isEmpty()) {
+ return null;
+ }
+ Seq<?> arguments = call.args();
+ Date value;
+ if (arguments.isEmpty()) {
+ value = currentValue(name);
+ } else if (arguments.size() == 1) {
+ String literal = literalOf(arguments.head());
+ if (literal == null) {
+ return null;
+ }
+ value = parse(name, literal);
+ if (value == null) {
+ return null;
+ }
+ } else {
+ return null;
+ }
+ String binding = SYNTHETIC_PREFIX + name + '.' + counter++;
+ this.bindings.put(binding, value);
+ return new Parameter(binding, AnyType.instance(), call.position());
+ }
+
+ private String literalOf(Object argument) {
+ if (argument instanceof StringLiteral) {
+ return ((StringLiteral) argument).value();
+ }
+ if (argument instanceof Parameter) {
+ Object bound = this.parameters.get(((Parameter) argument).name());
+ if (bound instanceof String) {
+ return (String) bound;
+ }
+ }
+ return null;
+ }
+
+ private static Date currentValue(String name) {
+ if (DATE.equals(name)) {
+ return Date.from(LocalDate.now().atStartOfDay(ZoneId.systemDefault()).toInstant());
+ }
+ return new Date();
+ }
+
+ private static Date parse(String name, String literal) {
+ String[] patterns = DATE.equals(name) ? DATE_FORMATS : DATETIME_FORMATS;
+ for (String pattern : patterns) {
+ SimpleDateFormat format = new SimpleDateFormat(pattern, Locale.ROOT);
Review Comment:
Important: `SimpleDateFormat` treats `SSS` as a count of milliseconds, not a fraction of a second, so `datetime('2024-02-05T10:15:30.5')` folds to 10:15:30.005 and `.12` folds to .012. Cypher reads both as fractions (.500 and .120), and so does HugeGraph's own DATE parsing: `DateUtil.parse("2024-02-05 10:15:30.5")` goes through Joda `yyyy-MM-dd HH:mm:ss.SSS` and stores 500 ms. I ran this method's parse step on JDK 17 and got 1, 12, 123 and 5 ms for `.1`, `.12`, `.123` and `.5`, while Joda gave 500 ms for `.5` and 120 ms for `.12`. So `MATCH (n) WHERE n.t = datetime('2024-02-05T10:15:30.5')` silently returns no rows for a vertex written as `2024-02-05 10:15:30.5`, and a `CREATE` writes the wrong instant. The existing test only uses a three-digit `.123`, the one length where both readings agree.
Please parse with `java.time` instead, for example `LocalDate.parse(literal).atStartOfDay(zone)` for `date` and `LocalDateTime.parse(literal).atZone(zone)` for the other two (catch `DateTimeParseException` and return `null`). That reads `.5` as 500 ms and `.12` as 120 ms, and as a side effect it also rejects `2024-2-5`, which the non-lenient `SimpleDateFormat` currently accepts but Cypher does not. Please add a test with a one- or two-digit fraction.
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4305308335
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,393 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ // Two leading spaces keep the prefix away from ordinary parameter names;
+ // backtick-escaped Cypher parameters can still carry it, so generated
+ // names skip every one the request already occupies.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
Review Comment:
Important: Only `statement` and `parameters` are replaced here. `CypherAst` also keeps `expressionTypes`, a `Map[Expression, CypherType]` built from the parsed statement, and the walkers look types up by structural equality. Every expression that `rebuild` copies now holds a `Parameter` where the original held a `FunctionInvocation`, so `expressionTypes.get` returns nothing for it. `ProjectionWalker.finalizeProjection` uses `qualifiedType(expression)` to decide whether a RETURN item needs `valueMap()`, so a node or relationship item that contains a folded call is returned as a raw element. `getReturnTypes` was computed at parse time and still says `Node`, so `ReturnNormalizer.normalizeElement` casts the element to `Map` and throws.
I ran parse, fold, translate and `ReturnNormalizer` at fe5daa6d on a TinkerGraph 3.8.1 graph:
- `MATCH (n) RETURN CASE WHEN n.d > date('2000-01-01') THEN n END AS m` throws `ClassCastException: TinkerVertex cannot be cast to java.util.Map`. The same query with `$d` bound to a `Date` returns `{m={_type=node, ...}}`.
- `MATCH (n) RETURN collect(CASE WHEN n.d > date('2000-01-01') THEN n END) AS ns` throws the same exception.
- `MATCH (a)-[r]->(b) RETURN CASE WHEN r.since > date('2000-01-01') THEN r END AS rel` throws `TinkerEdge cannot be cast to java.util.Map`.
The new docs say the constructors work wherever a bound value is accepted. Here the request fails with an internal cast error instead of a result, or the old `Unknown function` message.
Please carry the types over to the rebuilt nodes before the swap. For example, record each `original -> copy` pair for `Expression` nodes in an `IdentityHashMap` inside `copy()`, then read the `expressionTypes` field the same way as `statement` and add `copy -> type` for every original that has a type. I tried exactly that on this head: all three queries above return the same rows as their `$d` versions, and the 18 unit tests still pass. Please also add a unit test that checks the CASE query translates with `valueMap`, like its `$d` version does.
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4306461065
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,435 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.IdentityHashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.Expression;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.opencypher.v9_0.util.symbols.CypherType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ // Two leading spaces keep the prefix away from ordinary parameter names;
+ // backtick-escaped Cypher parameters can still carry it, so generated
+ // names skip every one the request already occupies.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed. The inferred types of the rebuilt expressions
+ * are carried over with the statement.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ Field typesField = CypherAst.class.getDeclaredField("expressionTypes");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+ typesField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ Object types = carryTypes(typesField.get(ast), fold.copies);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ typesField.set(ast, types);
+ } catch (Exception | LinkageError e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ /**
+ * Returns the expression type map extended with the types of the rebuilt
+ * expressions, so a statement the fold touched still finds the type of
+ * every expression, for example the valueMap() decision for a RETURN
+ * item that holds a node or relationship.
+ */
+ @SuppressWarnings("unchecked")
+ private static Object carryTypes(Object types, Map<Object, Object> copies) {
+ scala.collection.immutable.Map<Expression, CypherType> expressionTypes =
+ (scala.collection.immutable.Map<Expression, CypherType>) types;
+ scala.collection.immutable.Map<Expression, CypherType> carried = expressionTypes;
+ for (Map.Entry<Object, Object> pair : copies.entrySet()) {
+ if (pair.getKey() == pair.getValue()) {
+ continue;
+ }
+ Option<CypherType> type = expressionTypes.get((Expression) pair.getKey());
+ if (type.isDefined()) {
+ carried = carried.$plus(new Tuple2<>((Expression) pair.getValue(), type.get()));
+ }
+ }
+ return carried;
+ }
+
+ /**
+ * Returns a copy of the given parameters in which folded temporal values
+ * are replaced by a printable constructor form, so the string translation
+ * stays executable Groovy instead of rendering a date through toString.
+ */
+ public static Map<String, Object> explainParameters(Map<String, Object> parameters) {
+ boolean folded = false;
+ for (String name : parameters.keySet()) {
Review Comment:
⚠️ Important. Blocking: no. Summary: `evalCypher` calls `fold()` for every request, and this line starts the recursive AST walk before checking whether any synthetic binding was added. When nothing folds, the traversal still records expression mappings and allocates temporary sequence/set lists. Please avoid this work when no supported temporal constructor occurs, or add benchmark evidence showing the added latency and allocation cost is acceptable. Evidence: static trace through `TemporalFold.copy()`, `copySeq()` and `copySet()` on this head; no performance benchmark was run.
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4305160070
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ // Two leading spaces: parameter names coming from a Cypher query can
+ // never collide with this prefix, like the translator's own reserved names.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ } catch (Exception | LinkageError e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ private static final class TemporalFold {
+
+ private final Map<String, Object> parameters;
+ private final Map<String, Object> bindings = new LinkedHashMap<>();
+ private int counter = 0;
+
+ private TemporalFold(Map<String, Object> parameters) {
+ this.parameters = parameters;
+ }
+
+ /**
+ * Returns a copy of the node with temporal constructors folded in, or
+ * the same instance when the subtree does not change. Subtrees that
+ * cannot be rebuilt are shared with the original statement, so the
+ * copy stays minimal.
+ */
+ private Object copy(Object node) throws Exception {
+ if (node == null || node instanceof String || node instanceof Number ||
+ node instanceof Boolean || node instanceof Character) {
+ return node;
+ }
+ if (node instanceof FunctionInvocation) {
+ Object folded = foldCall((FunctionInvocation) node);
+ if (folded != null) {
+ return folded;
+ }
+ }
+ if (node instanceof Option) {
+ Option<?> option = (Option<?>) node;
+ if (!option.isEmpty()) {
+ Object value = option.get();
+ Object copied = copy(value);
+ if (copied != value) {
+ return new Some<>(copied);
+ }
+ }
+ return node;
+ }
+ if (node instanceof scala.collection.IndexedSeq) {
+ return copyIndexedSeq((scala.collection.IndexedSeq<?>) node);
+ }
+ if (node instanceof Seq) {
+ return copySeq((Seq<?>) node);
+ }
+ if (node instanceof scala.collection.immutable.Set) {
+ return copySet((scala.collection.immutable.Set<?>) node);
+ }
+ if (node instanceof Tuple2) {
+ Tuple2<?, ?> tuple = (Tuple2<?, ?>) node;
+ Object first = copy(tuple._1());
+ Object second = copy(tuple._2());
+ return first == tuple._1() && second == tuple._2() ? node : new Tuple2<>(first, second);
+ }
+ if (node instanceof Product) {
+ return rebuild((Product) node);
+ }
+ return node;
+ }
+
+ private Object foldCall(FunctionInvocation call) {
+ String name = call.name();
+ if (!DATE.equals(name) && !DATETIME.equals(name) && !LOCALDATETIME.equals(name)) {
Review Comment:
Important: This match is case-sensitive, but Cypher function names are not. `FunctionInvocation` resolves the function with `Function.lookup.getOrElse(name.toLowerCase, ...)` (expressions-9.0), and the translator matches with `fnName.toLowerCase match` (`ExpressionWalker.scala:221`, `ProjectionWalker.scala:360`), so `COUNT(n)` or `toUpper` in any case already work. `call.name()` keeps the case the user typed, so only the lowercase spelling folds. I compiled this file at c983b270 against translation 1.0.4 and ran parse, fold and translate: `MATCH (n) WHERE n.d = date('2000-01-01') RETURN n` folds, while `DATE('2000-01-01')`, `Date('2000-01-01')`, `DateTime('2024-02-05T10:15:30')` and `localDateTime('2024-02-05T10:15:30')` all still throw `SyntaxException: Unknown function 'DATE'` (or `'Date'`, `'DateTime'`, `'localDateTime'`). The docs added in this PR say these constructors now work, so a user who writes them in another case gets the old error.
Please normalize the name once, for example `String name = call.name().toLowerCase(Locale.ROOT);`, before this check, so `currentValue`, `parse` and the binding name all see the lowercase form. Please also add a unit test with an upper-case and a mixed-case call.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherOpProcessor.java:
##########
@@ -200,6 +200,9 @@ private void evalCypher(Context context) throws OpProcessorException {
ProcedureContext procedureContext = ProcedureContext.global();
CypherAst ast = CypherAst.parse(cypher, parameters, procedureContext.getSignatures());
validateParameters(ast, parameters);
+ // The translator has no temporal functions; fold the supported
+ // constructors into bound values before it rejects them.
+ parameters = CypherTemporalFunctions.fold(ast, parameters);
Review Comment:
Minor: The returned map also feeds `TranslationWriter.write(ir, strTranslator, parameters)` a few lines below. With `inline_parameters`, the Groovy writer prints a `java.util.Date` through `toString()`, so `EXPLAIN MATCH (n) WHERE n.d = date('2000-01-01') RETURN n` returns a `translation` containing `__.constant(Sat Jan 01 00:00:00 IST 2000)`. That is not valid Gremlin-Groovy, it drops the milliseconds, and it uses an ambiguous zone abbreviation. The IN-list form shows the same thing: `has('d', within(Sat Jan 01 00:00:00 IST 2000, Mon Jan 01 00:00:00 IST 2001))`. Execution is not affected, because the traversal translator gets the real `Date` objects. Please either keep the synthetic bindings out of the string translation or render them as an explicit constructor such as `new Date(946665000000L)`, and mention in `docs/cypher-api.md` that EXPLAIN shows folded values in that form.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ // Two leading spaces: parameter names coming from a Cypher query can
+ // never collide with this prefix, like the translator's own reserved names.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ } catch (Exception | LinkageError e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ private static final class TemporalFold {
+
+ private final Map<String, Object> parameters;
+ private final Map<String, Object> bindings = new LinkedHashMap<>();
+ private int counter = 0;
+
+ private TemporalFold(Map<String, Object> parameters) {
+ this.parameters = parameters;
+ }
+
+ /**
+ * Returns a copy of the node with temporal constructors folded in, or
+ * the same instance when the subtree does not change. Subtrees that
+ * cannot be rebuilt are shared with the original statement, so the
+ * copy stays minimal.
+ */
+ private Object copy(Object node) throws Exception {
+ if (node == null || node instanceof String || node instanceof Number ||
+ node instanceof Boolean || node instanceof Character) {
+ return node;
+ }
+ if (node instanceof FunctionInvocation) {
+ Object folded = foldCall((FunctionInvocation) node);
+ if (folded != null) {
+ return folded;
+ }
+ }
+ if (node instanceof Option) {
+ Option<?> option = (Option<?>) node;
+ if (!option.isEmpty()) {
+ Object value = option.get();
+ Object copied = copy(value);
+ if (copied != value) {
+ return new Some<>(copied);
+ }
+ }
+ return node;
+ }
+ if (node instanceof scala.collection.IndexedSeq) {
+ return copyIndexedSeq((scala.collection.IndexedSeq<?>) node);
+ }
+ if (node instanceof Seq) {
+ return copySeq((Seq<?>) node);
+ }
+ if (node instanceof scala.collection.immutable.Set) {
+ return copySet((scala.collection.immutable.Set<?>) node);
+ }
+ if (node instanceof Tuple2) {
+ Tuple2<?, ?> tuple = (Tuple2<?, ?>) node;
+ Object first = copy(tuple._1());
+ Object second = copy(tuple._2());
+ return first == tuple._1() && second == tuple._2() ? node : new Tuple2<>(first, second);
+ }
+ if (node instanceof Product) {
+ return rebuild((Product) node);
+ }
+ return node;
+ }
+
+ private Object foldCall(FunctionInvocation call) {
+ String name = call.name();
+ if (!DATE.equals(name) && !DATETIME.equals(name) && !LOCALDATETIME.equals(name)) {
+ return null;
+ }
+ if (call.distinct() || !call.namespace().parts().isEmpty()) {
+ return null;
+ }
+ Seq<?> arguments = call.args();
+ Date value;
+ if (arguments.isEmpty()) {
+ value = currentValue(name);
+ } else if (arguments.size() == 1) {
+ String literal = literalOf(arguments.head());
+ if (literal == null) {
+ return null;
+ }
+ value = parse(name, literal);
+ if (value == null) {
+ return null;
+ }
+ } else {
+ return null;
+ }
+ String binding = SYNTHETIC_PREFIX + name + '.' + counter++;
+ this.bindings.put(binding, value);
+ return new Parameter(binding, AnyType.instance(), call.position());
+ }
+
+ private String literalOf(Object argument) {
+ if (argument instanceof StringLiteral) {
+ return ((StringLiteral) argument).value();
+ }
+ if (argument instanceof Parameter) {
+ Object bound = this.parameters.get(((Parameter) argument).name());
+ if (bound instanceof String) {
+ return (String) bound;
+ }
+ }
+ return null;
+ }
+
+ private static Date currentValue(String name) {
+ if (DATE.equals(name)) {
+ return Date.from(LocalDate.now().atStartOfDay(ZoneId.systemDefault()).toInstant());
+ }
+ return new Date();
Review Comment:
Minor: Each no-argument call reads the clock on its own, so two `datetime()` or `localdatetime()` calls in one statement can fold to different instants. In Cypher the no-argument form uses the statement clock, so `RETURN datetime() = datetime()` is always true and `CREATE (a {t: datetime()}), (b {t: datetime()})` writes the same value twice. Running parse and fold of that CREATE 20,000 times on this head gave two different bindings in 18 runs. `date()` has the same problem across midnight. Please read the clock once per fold, for example capture an `Instant` in the `TemporalFold` constructor and derive both the `date()` and the `datetime()` values from 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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "bitflicker64 (via GitHub)" <gi...@apache.org>.
bitflicker64 commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4302861770
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/cypher/CypherTemporalFunctionTest.java:
##########
@@ -0,0 +1,161 @@
+/*
+ * 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.hugegraph.unit.api.cypher;
+
+import java.text.ParseException;
+import java.text.SimpleDateFormat;
+import java.time.LocalDate;
+import java.time.ZoneId;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+
+import org.apache.hugegraph.opencypher.CypherTemporalFunctions;
+import org.junit.Assert;
+import org.junit.Test;
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.gremlin.translation.translator.Translator;
+
+public class CypherTemporalFunctionTest {
Review Comment:
Important: This class is never run by the build. The `unit-test` profile in `hugegraph-test/pom.xml` includes only `**/UnitTestSuite.java` (`run-unit-test.sh` runs `mvn test ... -P unit-test`), and `UnitTestSuite` lists `CypherErrorTest.class` under `/* api cypher */` but not `CypherTemporalFunctionTest.class`. None of these 9 tests runs in CI. None of them puts a folded call under an `Ands` either, so they would not have caught the `copySet` failure. Please add `CypherTemporalFunctionTest.class` next to `CypherErrorTest.class` in `UnitTestSuite`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,338 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.text.ParsePosition;
+import java.text.SimpleDateFormat;
+import java.time.LocalDate;
+import java.time.ZoneId;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ private static final String[] DATE_FORMATS = {"yyyy-MM-dd"};
+ private static final String[] DATETIME_FORMATS = {"yyyy-MM-dd'T'HH:mm:ss",
+ "yyyy-MM-dd'T'HH:mm:ss.SSS"};
+
+ // Two leading spaces: parameter names coming from a Cypher query can
+ // never collide with this prefix, like the translator's own reserved names.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ } catch (Exception e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ private static final class TemporalFold {
+
+ private final Map<String, Object> parameters;
+ private final Map<String, Object> bindings = new LinkedHashMap<>();
+ private int counter = 0;
+
+ private TemporalFold(Map<String, Object> parameters) {
+ this.parameters = parameters;
+ }
+
+ /**
+ * Returns a copy of the node with temporal constructors folded in, or
+ * the same instance when the subtree does not change. Subtrees that
+ * cannot be rebuilt are shared with the original statement, so the
+ * copy stays minimal.
+ */
+ private Object copy(Object node) throws Exception {
+ if (node == null || node instanceof String || node instanceof Number ||
+ node instanceof Boolean || node instanceof Character) {
+ return node;
+ }
+ if (node instanceof FunctionInvocation) {
+ Object folded = foldCall((FunctionInvocation) node);
+ if (folded != null) {
+ return folded;
+ }
+ }
+ if (node instanceof Option) {
+ Option<?> option = (Option<?>) node;
+ if (!option.isEmpty()) {
+ Object value = option.get();
+ Object copied = copy(value);
+ if (copied != value) {
+ return new Some<>(copied);
+ }
+ }
+ return node;
+ }
+ if (node instanceof scala.collection.IndexedSeq) {
+ return copyIndexedSeq((scala.collection.IndexedSeq<?>) node);
+ }
+ if (node instanceof Seq) {
+ return copySeq((Seq<?>) node);
+ }
+ if (node instanceof scala.collection.immutable.Set) {
+ return copySet((scala.collection.immutable.Set<?>) node);
+ }
+ if (node instanceof Tuple2) {
+ Tuple2<?, ?> tuple = (Tuple2<?, ?>) node;
+ Object first = copy(tuple._1());
+ Object second = copy(tuple._2());
+ return first == tuple._1() && second == tuple._2() ? node : new Tuple2<>(first, second);
+ }
+ if (node instanceof Product) {
+ return rebuild((Product) node);
+ }
+ return node;
+ }
+
+ private Object foldCall(FunctionInvocation call) {
+ String name = call.name();
+ if (!DATE.equals(name) && !DATETIME.equals(name) && !LOCALDATETIME.equals(name)) {
+ return null;
+ }
+ if (call.distinct() || !call.namespace().parts().isEmpty()) {
+ return null;
+ }
+ Seq<?> arguments = call.args();
+ Date value;
+ if (arguments.isEmpty()) {
+ value = currentValue(name);
+ } else if (arguments.size() == 1) {
+ String literal = literalOf(arguments.head());
+ if (literal == null) {
+ return null;
+ }
+ value = parse(name, literal);
+ if (value == null) {
+ return null;
+ }
+ } else {
+ return null;
+ }
+ String binding = SYNTHETIC_PREFIX + name + '.' + counter++;
+ this.bindings.put(binding, value);
+ return new Parameter(binding, AnyType.instance(), call.position());
+ }
+
+ private String literalOf(Object argument) {
+ if (argument instanceof StringLiteral) {
+ return ((StringLiteral) argument).value();
+ }
+ if (argument instanceof Parameter) {
+ Object bound = this.parameters.get(((Parameter) argument).name());
+ if (bound instanceof String) {
+ return (String) bound;
+ }
+ }
+ return null;
+ }
+
+ private static Date currentValue(String name) {
+ if (DATE.equals(name)) {
+ return Date.from(LocalDate.now().atStartOfDay(ZoneId.systemDefault()).toInstant());
+ }
+ return new Date();
+ }
+
+ private static Date parse(String name, String literal) {
+ String[] patterns = DATE.equals(name) ? DATE_FORMATS : DATETIME_FORMATS;
+ for (String pattern : patterns) {
+ SimpleDateFormat format = new SimpleDateFormat(pattern, Locale.ROOT);
+ format.setLenient(false);
+ // Require the literal to match the pattern completely, so a
+ // shorter pattern cannot silently drop a trailing part.
+ ParsePosition position = new ParsePosition(0);
+ Date parsed = format.parse(literal, position);
+ if (parsed != null && position.getIndex() == literal.length()) {
+ return parsed;
+ }
+ }
+ return null;
+ }
+
+ private Object copyIndexedSeq(scala.collection.IndexedSeq<?> seq) throws Exception {
+ List<Object> copies = new ArrayList<>(seq.size());
+ boolean changed = false;
+ scala.collection.Iterator<?> iterator = seq.iterator();
+ while (iterator.hasNext()) {
+ Object element = iterator.next();
+ Object copied = copy(element);
+ changed |= copied != element;
+ copies.add(copied);
+ }
+ if (!changed) {
+ return seq;
+ }
+ return scala.collection.immutable.IndexedSeq$.MODULE$.apply(JavaConverters.asScalaBuffer(copies));
+ }
+
+ private Object copySeq(Seq<?> seq) throws Exception {
+ List<Object> copies = new ArrayList<>(seq.size());
+ boolean changed = false;
+ scala.collection.Iterator<?> iterator = seq.iterator();
+ while (iterator.hasNext()) {
+ Object element = iterator.next();
+ Object copied = copy(element);
+ changed |= copied != element;
+ copies.add(copied);
+ }
+ if (!changed) {
+ return seq;
+ }
+ return JavaConverters.asScalaBuffer(copies).toList();
+ }
+
+ private Object copySet(scala.collection.immutable.Set<?> set) throws Exception {
+ List<Object> copies = new ArrayList<>(set.size());
+ boolean changed = false;
+ scala.collection.Iterator<?> iterator = set.iterator();
+ while (iterator.hasNext()) {
+ Object element = iterator.next();
+ Object copied = copy(element);
+ changed |= copied != element;
+ copies.add(copied);
+ }
+ if (!changed) {
+ return set;
+ }
+ scala.collection.Set<Object> result = scala.collection.immutable.Set$.MODULE$.empty();
+ for (Object element : copies) {
+ result = result.$plus(element);
Review Comment:
Important: This call fails at runtime with `NoSuchMethodError: 'scala.collection.SetLike scala.collection.Set.$plus(java.lang.Object)'`. javac reads Scala's generic signature for `SetLike.+` and emits `invokeinterface scala/collection/Set.$plus:(Ljava/lang/Object;)Lscala/collection/SetLike;`, but scala-library 2.12.7 (the version HugeGraph ships) only has `$plus:(Ljava/lang/Object;)Lscala/collection/Set;`. So any fold under a Scala `Set` breaks, which means every `Ands`: a labelled `MATCH ... WHERE`, a labelled node with an inline property map, or any `AND`.
I compiled this file at c2ca933 against `translation:1.0.4` and ran `CypherAst.parse` + `fold`. These throw:
- `MATCH (n:cypher_person) WHERE n.birthday = date('2000-01-01') RETURN n.name AS name` (the new `testDateFunctionMatchesDateProperty`)
- `MATCH (n:cypher_person {note: date('2024-02-05')}) RETURN count(n) AS count` (the new `testDateFunctionYieldsNoMatchOnTextProperty`)
- `MATCH (n) WHERE n.d0 > date('2024-02-05') AND n.d0 < date('2025-01-01') RETURN n`
Only unlabelled single-predicate queries such as `MATCH (n {d0: date('2024-02-05')})` fold. Because this is an `Error`, the `catch (Exception e)` in `fold()` does not catch it. The documented fallback to the old `Unknown function` error never happens, and the error propagates out of `evalCypher`.
Please build the set without `$plus`, for example replace lines 291-295 with `return JavaConverters.asScalaBuffer(copies).toSet();`. With that change all of the queries above fold and translate, and the 9 existing unit tests still pass. Please also widen the fallback catch to include `LinkageError`, so a reflective mismatch degrades as the Javadoc promises, and add a unit test with a labelled MATCH and an `AND` range.
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] fix(api): fold Cypher temporal constructors into bound date values [hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #3370:
URL: https://github.com/apache/hugegraph/pull/3370#discussion_r4305155194
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java:
##########
@@ -0,0 +1,330 @@
+/*
+ * 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.hugegraph.opencypher;
+
+import static org.slf4j.LoggerFactory.getLogger;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.time.format.DateTimeParseException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+
+import org.opencypher.gremlin.translation.CypherAst;
+import org.opencypher.v9_0.expressions.FunctionInvocation;
+import org.opencypher.v9_0.expressions.Parameter;
+import org.opencypher.v9_0.expressions.StringLiteral;
+import org.opencypher.v9_0.util.InputPosition;
+import org.opencypher.v9_0.util.symbols.AnyType;
+import org.slf4j.Logger;
+
+import scala.Option;
+import scala.Product;
+import scala.Some;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.Seq;
+
+/**
+ * Rewrites the Cypher temporal constructor functions that the cypher-for-gremlin
+ * translator does not implement into parameters bound to native date values.
+ * <p>
+ * The translator's function list is fixed inside its own dependency, so the
+ * supported constructors (date, datetime, localdatetime) are folded into the
+ * parsed statement right before translation: each call with a foldable argument
+ * becomes a synthetic bound parameter and the date value is added to the
+ * bindings. Calls that cannot be folded are left in place and keep failing
+ * with the translator's original error, as do all other temporal functions.
+ */
+public final class CypherTemporalFunctions {
+
+ private static final Logger logger = getLogger(CypherTemporalFunctions.class);
+
+ private static final String DATE = "date";
+ private static final String DATETIME = "datetime";
+ private static final String LOCALDATETIME = "localdatetime";
+
+ // Two leading spaces: parameter names coming from a Cypher query can
+ // never collide with this prefix, like the translator's own reserved names.
+ private static final String SYNTHETIC_PREFIX = " cypher.fold.";
+
+ private CypherTemporalFunctions() {
+ }
+
+ /**
+ * Folds the supported temporal constructors in the parsed statement and
+ * returns the effective request parameters for translation: the same
+ * instance when nothing was folded, a copy carrying the synthetic
+ * bindings otherwise. The parsed statement is only replaced when the
+ * rewrite completed without errors, so a fold failure leaves the query
+ * exactly as it was parsed.
+ */
+ public static Map<String, Object> fold(CypherAst ast, Map<String, Object> parameters) {
+ TemporalFold fold = new TemporalFold(parameters);
+ try {
+ Field statementField = CypherAst.class.getDeclaredField("statement");
+ Field parametersField = CypherAst.class.getDeclaredField("parameters");
+ statementField.setAccessible(true);
+ parametersField.setAccessible(true);
+
+ Object folded = fold.copy(ast.statement());
+ if (fold.bindings.isEmpty()) {
+ return parameters;
+ }
+ Object merged = mergeParameters(parametersField.get(ast), fold.bindings);
+ statementField.set(ast, folded);
+ parametersField.set(ast, merged);
+ } catch (Exception | LinkageError e) {
+ logger.warn("Failed to fold temporal functions, keeping the parsed statement", e);
+ return parameters;
+ }
+ Map<String, Object> effective = new LinkedHashMap<>(parameters);
+ effective.putAll(fold.bindings);
+ return effective;
+ }
+
+ @SuppressWarnings("unchecked")
+ private static Object mergeParameters(Object parameters, Map<String, Object> bindings) {
+ scala.collection.immutable.Map<String, Object> merged =
+ (scala.collection.immutable.Map<String, Object>) parameters;
+ for (Map.Entry<String, Object> binding : bindings.entrySet()) {
+ merged = merged.$plus(new Tuple2<>(binding.getKey(), binding.getValue()));
+ }
+ return merged;
+ }
+
+ private static final class TemporalFold {
+
+ private final Map<String, Object> parameters;
+ private final Map<String, Object> bindings = new LinkedHashMap<>();
+ private int counter = 0;
+
+ private TemporalFold(Map<String, Object> parameters) {
+ this.parameters = parameters;
+ }
+
+ /**
+ * Returns a copy of the node with temporal constructors folded in, or
+ * the same instance when the subtree does not change. Subtrees that
+ * cannot be rebuilt are shared with the original statement, so the
+ * copy stays minimal.
+ */
+ private Object copy(Object node) throws Exception {
+ if (node == null || node instanceof String || node instanceof Number ||
+ node instanceof Boolean || node instanceof Character) {
+ return node;
+ }
+ if (node instanceof FunctionInvocation) {
+ Object folded = foldCall((FunctionInvocation) node);
+ if (folded != null) {
+ return folded;
+ }
+ }
+ if (node instanceof Option) {
+ Option<?> option = (Option<?>) node;
+ if (!option.isEmpty()) {
+ Object value = option.get();
+ Object copied = copy(value);
+ if (copied != value) {
+ return new Some<>(copied);
+ }
+ }
+ return node;
+ }
+ if (node instanceof scala.collection.IndexedSeq) {
+ return copyIndexedSeq((scala.collection.IndexedSeq<?>) node);
+ }
+ if (node instanceof Seq) {
+ return copySeq((Seq<?>) node);
+ }
+ if (node instanceof scala.collection.immutable.Set) {
+ return copySet((scala.collection.immutable.Set<?>) node);
+ }
+ if (node instanceof Tuple2) {
+ Tuple2<?, ?> tuple = (Tuple2<?, ?>) node;
+ Object first = copy(tuple._1());
+ Object second = copy(tuple._2());
+ return first == tuple._1() && second == tuple._2() ? node : new Tuple2<>(first, second);
+ }
+ if (node instanceof Product) {
+ return rebuild((Product) node);
+ }
+ return node;
+ }
+
+ private Object foldCall(FunctionInvocation call) {
+ String name = call.name();
+ if (!DATE.equals(name) && !DATETIME.equals(name) && !LOCALDATETIME.equals(name)) {
+ return null;
+ }
+ if (call.distinct() || !call.namespace().parts().isEmpty()) {
+ return null;
+ }
+ Seq<?> arguments = call.args();
+ Date value;
+ if (arguments.isEmpty()) {
+ value = currentValue(name);
+ } else if (arguments.size() == 1) {
+ String literal = literalOf(arguments.head());
+ if (literal == null) {
+ return null;
+ }
+ value = parse(name, literal);
+ if (value == null) {
+ return null;
+ }
+ } else {
+ return null;
+ }
+ String binding = SYNTHETIC_PREFIX + name + '.' + counter++;
+ this.bindings.put(binding, value);
+ return new Parameter(binding, AnyType.instance(), call.position());
Review Comment:
[P2] Avoid overwriting existing escaped parameter names
The two-space prefix is expressible using a backtick-escaped Cypher parameter:
```cypher
RETURN date('2024-02-05') AS d, $` cypher.fold.date.0` AS other
```
Parameters:
```json
{" cypher.fold.date.0":"original"}
```
The actual parser accepts the query, but folding replaces the existing binding with the constructed Date, so both translated output bindings use that Date instead of preserving `original` for `other`. The same parameter without a temporal constructor remains unchanged. `CypherOpProcessor` accepts string keys and `ParameterNormalizer` preserves this name.
Generate synthetic names by checking request parameters and accumulated synthetic bindings and skipping occupied names; add literal-constructor and parameter-constructor collision regressions.
Evidence: independently reproduced by two reviewers on the actual parser/fold/translation/binding paths at [these lines](https://github.com/apache/hugegraph/blob/c983b270f4a633207ebd4bd08e160106c1a8b8bc/hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/opencypher/CypherTemporalFunctions.java#L200-L202). No live database execution is claimed.
--
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@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org