spark-test commented on code in PR #59908:
URL: https://github.com/apache/spark/pull/59908#discussion_r4418114502
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/connector/ToStringSQLBuilder.scala:
##########
@@ -58,6 +59,7 @@ class ToStringSQLBuilder extends V2ExpressionSQLBuilder with
Serializable {
// until StackOverflowError. Render it as a function call instead.
override protected def visitUnexpectedExpr(expr: Expression): String = expr
match {
case e: GeneralScalarExpression => visitSQLFunction(e.name(), e.children())
+ case p: LookupParameter => p.describe()
Review Comment:
**Nit (P3):** With this case, `describe()` of an offered null-safe candidate
such as `id <=> ?` goes through the inherited `<=>` expansion in
`V2ExpressionSQLBuilder.visitBinaryComparison`, which writes each operand three
times: `((id IS NOT NULL AND ? IS NOT NULL AND id = ?) OR (id IS NULL AND ? IS
NULL))`. That shows three `?` for one parameter, and `[id <=> ?, ts >= ?]`
prints four for a two-field parameter row. For a literal the repetition is
harmless because the value is visible; here it misstates how many parameter-row
fields the candidate contributes, in logs, error messages and any future
EXPLAIN. A narrow fix is to override the expression-level
`visitBinaryComparison(name, le, re)` in this builder, rendering `l <=> r` when
an operand is a `LookupParameter` and delegating to `super` otherwise (as
`OracleDialect` does for its own operand types). That keeps the expanded form
`V2PredicateSuite` pins for literal `<=>` filters. Only `id = ?` is rendered in
the new tests today.
**Verification:**
- **Behavior:** A null-safe lookup candidate between a column and a lookup
parameter is described with a single parameter marker, while equality and range
candidates keep their current single-marker rendering.
- **Compatibility:** The existing rendering of a null-safe comparison
between a column and a literal keeps its expanded IS NULL form, so
pushed-filter descriptions and the tests that pin them are unaffected.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/read/ExternalLookupApiSuite.scala:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.read
+
+import scala.reflect.ClassTag
+
+import org.apache.spark.{SparkConf, SparkFunSuite,
SparkUnsupportedOperationException}
+import org.apache.spark.serializer.JavaSerializer
+import org.apache.spark.sql.connector.expressions.{Expression, FieldReference}
+import org.apache.spark.sql.connector.expressions.filter.Predicate
+import org.apache.spark.sql.types.{IntegerType, StructType}
+
+class ExternalLookupApiSuite extends SparkFunSuite {
+
+ private val offeredPredicate: Predicate =
+ new Predicate("=", Array[Expression](
+ FieldReference.column("id"), new LookupParameter(IntegerType)))
+
+ private def javaRoundTrip[T: ClassTag](value: T): T = {
+ val serializer = new JavaSerializer(new SparkConf()).newInstance()
+ serializer.deserialize[T](serializer.serialize(value))
+ }
+
+ test("a lookup parameter is a leaf rendered as ?") {
Review Comment:
**Nit (P3):** `LookupParameter` documents identity equality ("same-shaped
candidates are distinct"), and that identity is the only thing that keeps two
same-shaped candidates apart: `Predicate` inherits
`GeneralScalarExpression.equals`/`hashCode`, which compare the name and
children. None of the tests in this suite compares two parameters or two
same-shaped predicates, so a later value-based `equals` (for consistency with
`FieldReference` or `LiteralValue`) would pass all of them and silently merge
candidates such as the two offered for `t.id = s.a AND t.id = s.b + 1`.
Equality assertions next to this test would pin the contract.
**Verification:**
- **Regression:** Two same-shaped lookup candidates built from distinct
lookup parameters compare unequal while each compares equal to itself, so
giving the lookup parameter value-based equality fails the tests. The
assertions compare with equals only; distinct objects are not guaranteed
distinct hash codes, so the tests do not assert hash-code inequality.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/ExternalLookupReader.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.spark.sql.connector.read;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.catalyst.InternalRow;
+import org.apache.spark.sql.connector.metric.CustomTaskMetric;
+
+/**
+ * A task-local client that executes lookup requests.
+ * <ul>
+ * <li>Calls to a reader and its cursors are sequential, but may come from
different threads.</li>
+ * <li>Requests are not snapshot boundaries: results may mix source
versions.</li>
+ * <li>Retried or speculative tasks may repeat a request; duplicates must be
tolerated.</li>
+ * <li>Implementations should respond to thread interruption if their client
supports it.</li>
+ * </ul>
+ *
+ * @since 4.4.0
+ */
+@Evolving
+public interface ExternalLookupReader extends Closeable {
+
+ /**
+ * Looks up {@code uniqueParameters} and returns a cursor over all matches.
For example:
+ * <pre>{@code
+ * uniqueParameters: [(1), (2), (3)]
+ * cursor yields: (0, a), (0, b), (2, c) // (inputOrdinal, value row)
+ * => row 0 has two matches, row 1 is a miss, row 2 has one match
+ * }</pre>
+ * Spark guarantees that {@code uniqueParameters} is non-null, non-empty and
has at most
+ * {@link ExternalLookup#maxParameterRowsPerRequest()} non-null rows. Field
{@code i} of each row
Review Comment:
**Nit (P3):** As written, this caps only how many elements are non-null: an
array with null slots after N non-null rows would satisfy it, and the array
length itself is never bounded. The rest of the contract treats every element
as a distinct parameter row, and `inputOrdinal()` ranges over `[0,
uniqueParameters.length)`, so implementors can't tell from this sentence
whether to handle null elements or whether they can size per-request buffers
from `maxParameterRowsPerRequest()`. Suggested wording: `Spark guarantees that
{@code uniqueParameters} is non-null and holds between 1 and {@link
ExternalLookup#maxParameterRowsPerRequest()} rows, none of them null.`
**Verification:**
- **Inspection:** The reworded guarantee bounds the length of the input
array by maxParameterRowsPerRequest(), rules out null elements, and stays
consistent with the cursor's input ordinal ranging over the whole array.
- **Compile:** The connector read API documentation still builds without
Javadoc lint errors and stays within the source line-length limit.
--
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]
