You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhengruifeng (via GitHub)" <gi...@apache.org> on 2024/02/27 05:23:19 UTC
[PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
zhengruifeng opened a new pull request, #45277:
URL: https://github.com/apache/spark/pull/45277
### What changes were proposed in this pull request?
Implement SQLStringFormatter for Python Client
### Why are the changes needed?
for parity
### Does this PR introduce _any_ user-facing change?
yes, new feature
```
In [1]: mydf = spark.range(10)
In [2]: spark.sql("SELECT {col} FROM {mydf} WHERE id IN {x}", col=mydf.id, mydf=mydf, x=tuple(range(4))).show()
+---+
| id|
+---+
| 0|
| 1|
| 2|
| 3|
+---+
```
### How was this patch tested?
enabled doc tests
### Was this patch authored or co-authored using generative AI tooling?
no
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #45277:
URL: https://github.com/apache/spark/pull/45277#issuecomment-1965815610
I found introducing a CTE message may make it more complex, since a `SQLCommand` is executed eagerly.
```
UnresolvedWith (CTE)
- SQLCommand
- cteRelations
```
So I switch to adding views fields in existing `SQLCommand`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "ueshin (via GitHub)" <gi...@apache.org>.
ueshin commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504862603
##########
python/pyspark/sql/connect/sql_formatter.py:
##########
@@ -0,0 +1,77 @@
+#
+# 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.
+#
+
+import string
+import typing
+from typing import Any, Optional, List, Tuple, Sequence, Mapping
+import uuid
+
+from pyspark.errors import PySparkValueError
+
+if typing.TYPE_CHECKING:
+ from pyspark.sql.connect.session import SparkSession
+ from pyspark.sql.connect.dataframe import DataFrame
+
+
+class SQLStringFormatter(string.Formatter):
+ """
+ A standard ``string.Formatter`` in Python that can understand PySpark instances
+ with basic Python objects. This object has to be clear after the use for single SQL
+ query; cannot be reused across multiple SQL queries without cleaning.
+ """
+
+ def __init__(self, session: "SparkSession") -> None:
+ self._session: "SparkSession" = session
+ self._temp_views: List[Tuple[DataFrame, str]] = []
+
+ def get_field(self, field_name: str, args: Sequence[Any], kwargs: Mapping[str, Any]) -> Any:
+ obj, first = super(SQLStringFormatter, self).get_field(field_name, args, kwargs)
+ return self._convert_value(obj, field_name), first
+
+ def _convert_value(self, val: Any, field_name: str) -> Optional[str]:
+ """
+ Converts the given value into a SQL string.
+ """
+ from pyspark.sql.connect.dataframe import DataFrame
+ from pyspark.sql.connect.column import Column
+ from pyspark.sql.connect.expressions import ColumnReference
+
+ if isinstance(val, Column):
+ expr = val._expr
+ if isinstance(expr, ColumnReference):
+ return expr._unparsed_identifier
+ else:
+ raise PySparkValueError(
+ "%s in %s should be a plain column reference such as `df.col` "
+ "or `col('column')`" % (val, field_name)
+ )
+ elif isinstance(val, DataFrame):
+ for df, n in self._temp_views:
+ if df is val:
+ return n
+ name = "_pyspark_connect_temp_view_%s" % str(uuid.uuid4()).replace("-", "")
+ self._temp_views.append((val, name))
+ return name
+ elif isinstance(val, str):
+ from pyspark.sql.utils import get_lit_sql_str
Review Comment:
ditto.
##########
python/pyspark/sql/sql_formatter.py:
##########
@@ -75,7 +74,9 @@ def _convert_value(self, val: Any, field_name: str) -> Optional[str]:
val.createOrReplaceTempView(df_name)
return df_name
elif isinstance(val, str):
- return lit(val)._jc.expr().sql() # for escaped characters.
+ from pyspark.sql.utils import get_lit_sql_str
Review Comment:
Seems to be ok to put at the header of this file?
##########
python/pyspark/sql/connect/session.py:
##########
@@ -574,13 +575,29 @@ def createDataFrame(
createDataFrame.__doc__ = PySparkSession.createDataFrame.__doc__
- def sql(self, sqlQuery: str, args: Optional[Union[Dict[str, Any], List]] = None) -> "DataFrame":
- cmd = SQL(sqlQuery, args)
- data, properties = self.client.execute_command(cmd.command(self._client))
+ def sql(
+ self,
+ sqlQuery: str,
+ args: Optional[Union[Dict[str, Any], List]] = None,
+ **kwargs: Any,
+ ) -> "DataFrame":
+ views: List[SubqueryAlias] = []
+ if len(kwargs) > 0:
+ from pyspark.sql.connect.sql_formatter import SQLStringFormatter
+
+ formatter = SQLStringFormatter(self)
+ sqlQuery = formatter.format(sqlQuery, **kwargs)
+
+ for temp_view in formatter._temp_views:
+ df, name = temp_view[0], temp_view[1]
Review Comment:
nit:
```suggestion
for df, name in formatter._temp_views:
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504278418
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/planner/SparkConnectPlanner.scala:
##########
@@ -2632,6 +2626,60 @@ class SparkConnectPlanner(
}
}
+ private def executeSQLCommand(
+ sqlCommand: SqlCommand,
+ tracker: QueryPlanningTracker = new QueryPlanningTracker) = {
+ if (sqlCommand.getViewsCount > 0) {
+ val views = sqlCommand.getViewsList.asScala
+ this.synchronized {
+ try {
+ views.foreach { view =>
Review Comment:
Are views allowed to reference each other?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504283724
##########
python/pyspark/sql/connect/sql_formatter.py:
##########
@@ -0,0 +1,77 @@
+#
+# 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.
+#
+
+import string
+import typing
+from typing import Any, Optional, List, Tuple, Sequence, Mapping
+import uuid
+
+from pyspark.errors import PySparkValueError
+
+if typing.TYPE_CHECKING:
+ from pyspark.sql.connect.session import SparkSession
+ from pyspark.sql.connect.dataframe import DataFrame
+
+
+class SQLStringFormatter(string.Formatter):
+ """
+ A standard ``string.Formatter`` in Python that can understand PySpark instances
+ with basic Python objects. This object has to be clear after the use for single SQL
+ query; cannot be reused across multiple SQL queries without cleaning.
+ """
+
+ def __init__(self, session: "SparkSession") -> None:
+ self._session: "SparkSession" = session
+ self._temp_views: List[Tuple[DataFrame, str]] = []
+
+ def get_field(self, field_name: str, args: Sequence[Any], kwargs: Mapping[str, Any]) -> Any:
+ obj, first = super(SQLStringFormatter, self).get_field(field_name, args, kwargs)
+ return self._convert_value(obj, field_name), first
+
+ def _convert_value(self, val: Any, field_name: str) -> Optional[str]:
+ """
+ Converts the given value into a SQL string.
+ """
+ from pyspark.sql.connect.dataframe import DataFrame
+ from pyspark.sql.connect.column import Column
+ from pyspark.sql.connect.expressions import ColumnReference
+
+ if isinstance(val, Column):
+ expr = val._expr
+ if isinstance(expr, ColumnReference):
+ return expr._unparsed_identifier
+ else:
+ raise PySparkValueError(
Review Comment:
Does this work in the non-connect version?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504306517
##########
python/pyspark/sql/utils.py:
##########
@@ -354,3 +354,11 @@ def get_window_class() -> Type["Window"]:
return ConnectWindow # type: ignore[return-value]
else:
return PySparkWindow
+
+
+def get_lit_sql_str(val: str) -> str:
+ # Equivalent to `lit(val)._jc.expr().sql()` for string typed val
+ # This is matched to behavior from JVM implementation.
+ # See `sql` definition from `sql/catalyst/src/main/scala/org/apache/spark/
+ # sql/catalyst/expressions/literals.scala`
+ return "'" + val.replace("\\", "\\\\").replace("'", "\\'") + "'"
Review Comment:
Is there a way we can make the two share tests?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504307792
##########
python/pyspark/sql/connect/sql_formatter.py:
##########
@@ -0,0 +1,77 @@
+#
+# 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.
+#
+
+import string
+import typing
+from typing import Any, Optional, List, Tuple, Sequence, Mapping
+import uuid
+
+from pyspark.errors import PySparkValueError
+
+if typing.TYPE_CHECKING:
+ from pyspark.sql.connect.session import SparkSession
+ from pyspark.sql.connect.dataframe import DataFrame
+
+
+class SQLStringFormatter(string.Formatter):
Review Comment:
Should we add tests here? If this is covered elsewhere then that also works.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng closed pull request #45277: [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter`
URL: https://github.com/apache/spark/pull/45277
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #45277:
URL: https://github.com/apache/spark/pull/45277#issuecomment-2046490077
close this PR in favor of https://github.com/apache/spark/pull/45614
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1504312333
##########
connector/connect/common/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -129,6 +129,9 @@ message SQL {
// (Optional) A sequence of expressions for positional parameters in the SQL query text.
// It cannot coexist with `named_arguments`.
repeated Expression pos_arguments = 5;
+
Review Comment:
I'd like to introduce a slightly more generic mechanism here. Where we define a CTE or something similar. This way we can fix recursion limits, and we can reduce tree duplication.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on code in PR #45277:
URL: https://github.com/apache/spark/pull/45277#discussion_r1505230904
##########
connector/connect/common/src/main/protobuf/spark/connect/relations.proto:
##########
@@ -129,6 +129,9 @@ message SQL {
// (Optional) A sequence of expressions for positional parameters in the SQL query text.
// It cannot coexist with `named_arguments`.
repeated Expression pos_arguments = 5;
+
Review Comment:
Sounds good, let me mark this PR wip for now
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-41811][PYTHON][CONNECT] Implement `SQLStringFormatter` [spark]
Posted by "zhengruifeng (via GitHub)" <gi...@apache.org>.
zhengruifeng commented on PR #45277:
URL: https://github.com/apache/spark/pull/45277#issuecomment-1965829385
@HyukjinKwon @MaxGekk @grundprinzip @hvanhovell would you mind taking a look? thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org