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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
