You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HyukjinKwon (via GitHub)" <gi...@apache.org> on 2024/05/29 11:02:33 UTC
[PR] [SPARK-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
HyukjinKwon opened a new pull request, #46789:
URL: https://github.com/apache/spark/pull/46789
### What changes were proposed in this pull request?
This PR proposes to Implement DataFrameQueryContext in Spark Connect. It adds two new protobuf messages packed together with `Expression`:
```proto
message Origin {
// (Required) Indicate the origin type.
oneof function {
PythonOrigin python_origin = 1;
}
}
message PythonOrigin {
// (Required) Name of the origin, for example, the name of the function
string fragment = 1;
// (Required) Callsite to show to end users, for example, stacktrace.
string call_site = 2;
}
```
They are set to individual expression messages, and use the information such as callsite when analysis happens. This resembles Spark SQL implementation.
See also https://github.com/apache/spark/pull/45377.
### Why are the changes needed?
See https://github.com/apache/spark/pull/45377
### Does this PR introduce _any_ user-facing change?
Yes, same as https://github.com/apache/spark/pull/45377 but in Spark Connect.
### How was this patch tested?
Same unittests reused in Spark Connect.
### 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1640213550
##########
python/pyspark/errors/utils.py:
##########
@@ -172,19 +181,38 @@ def _with_origin(func: Callable[..., Any]) -> Callable[..., Any]:
@functools.wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
from pyspark.sql import SparkSession
+ from pyspark.sql.utils import is_remote
spark = SparkSession.getActiveSession()
if spark is not None and hasattr(func, "__name__"):
- assert spark._jvm is not None
- pyspark_origin = spark._jvm.org.apache.spark.sql.catalyst.trees.PySparkCurrentOrigin
+ if is_remote():
+ global current_origin
- # Update call site when the function is called
- _capture_call_site(spark, pyspark_origin, func.__name__)
+ # Getting the configuration requires RPC call. Uses the default value for now.
+ depth = 1
Review Comment:
what's the purpose of depth?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643988916
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
.setObjectType(queryCtx.objectType())
.setObjectName(queryCtx.objectName())
.setStartIndex(queryCtx.startIndex())
.setStopIndex(queryCtx.stopIndex())
.setFragment(queryCtx.fragment())
- .build())
+ .setSummary(queryCtx.summary())
+ .build()
+ } else {
Review Comment:
For now, yes because we only have `QueryContextType.SQL` and `QueryContextType.DataFrame`.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643986906
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
.setObjectType(queryCtx.objectType())
.setObjectName(queryCtx.objectName())
.setStartIndex(queryCtx.startIndex())
.setStopIndex(queryCtx.stopIndex())
.setFragment(queryCtx.fragment())
- .build())
+ .setSummary(queryCtx.summary())
Review Comment:
Yes, so we did not have `QueryContext.sumary()` API before this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642558434
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Yeah, I think we should do. Let me take a look separately after this PR. This PR isn't related to the original implementation.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46789:
URL: https://github.com/apache/spark/pull/46789#issuecomment-2175453126
> The constructor on the DataFrame class could simply extract this from the call stack. In all honesty, this would probably be true as well for the PySpark classic class.
For this, I will revisit and see if I can fix Spark Classic together.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1640211627
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -405,3 +407,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
+ // (Required) Indicate the origin type.
+ oneof function {
+ PythonOrigin python_origin = 1;
+ }
+}
+
+message PythonOrigin {
Review Comment:
Why can't the Python code be modeled as a StackTraceElement? Whats the difference?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46789:
URL: https://github.com/apache/spark/pull/46789#issuecomment-2175508407
Merged to master.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641619279
##########
python/pyspark/errors/utils.py:
##########
@@ -172,19 +181,38 @@ def _with_origin(func: Callable[..., Any]) -> Callable[..., Any]:
@functools.wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
from pyspark.sql import SparkSession
+ from pyspark.sql.utils import is_remote
spark = SparkSession.getActiveSession()
if spark is not None and hasattr(func, "__name__"):
- assert spark._jvm is not None
- pyspark_origin = spark._jvm.org.apache.spark.sql.catalyst.trees.PySparkCurrentOrigin
+ if is_remote():
+ global current_origin
- # Update call site when the function is called
- _capture_call_site(spark, pyspark_origin, func.__name__)
+ # Getting the configuration requires RPC call. Uses the default value for now.
+ depth = 1
Review Comment:
It shows the maximum traceback of the error. For example, when it's `1`:
```
...
== DataFrame ==
"divide" was called from
/.../spark/python/test_pyspark_error.py:4
```
when it's 2:
```
...
== DataFrame ==
"divide" was called from
/.../spark/python/test_pyspark_error.py:4
/.../spark/python/test_parent_pyspark_error.py:5
```
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641621467
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
Re: https://github.com/apache/spark/pull/46789#discussion_r1640210979 (I think it's related).
The tricky thing is that `RelationCommon` binds to `Relation`. However, the lazy evaluation tracking is based on where the expressions were created ,e.g., before we create a `DataFrame`.
If we can store the callside of the individual expressions at `source_info` in `RelationCommon`, we could still be able to do it, but I think it's a little bit awkward probably because the current implementation is specific to `Expression`s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1618832199
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -405,3 +407,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
+ // (Required) Indicate the origin type.
+ oneof function {
+ PythonOrigin python_origin = 1;
+ }
+}
+
+message PythonOrigin {
Review Comment:
I actually intend this to be language specific. For example, Scala side could have stacktrace chain https://github.com/apache/spark/blob/8bbbde7cb3c396bc369c06853ed3a2ec021a2530/sql/api/src/main/scala/org/apache/spark/sql/catalyst/trees/QueryContexts.scala#L138
This is Python specifically a string 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641617771
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
The current implementation of tracking lazy evaluation is specific to `Expression`s actually (see `org.apache.spark.sql.withOrigin`, and Python client mimics the same approach which is way we're adding something to `Expression(self.origin)`.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642325778
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
hmm, is it a regression?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643986061
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
Review Comment:
Yeah .. so it has been always `SQLQueryContext` by default ...
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642396732
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Ah, this is actually a bug fix. Previously it was (wrongly) checking the Scala side `DataFrameQueryContext.fragment` which had to be `DataFrameQueryContext.pysparkFragement`. Now it correctly checks `DataFrameQueryContext.pysparkFragement`.
This is because we now merge `pysparkFragment` to `fragment` at `DataFrameQueryContext`, and it correctly tests the fragment.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642423116
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
In the scala side, we use the call site that is closest to the user code as the fragment. I don't think `__truediv__` is user-friendly.
For Java, we can find the latest stack trace that is from `org.apache.spark`, whose next trace is the user code. Can we do the same thing in Python?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1640215498
##########
python/pyspark/sql/connect/expressions.py:
##########
@@ -162,7 +172,7 @@ def __init__(self, parent: Expression, alias: Sequence[str], metadata: Any):
def to_plan(self, session: "SparkConnectClient") -> "proto.Expression":
if len(self._alias) == 1:
- exp = proto.Expression()
+ exp = proto.Expression(origin=self.origin)
Review Comment:
We should fix this repetitive code in a similar way we've done for relations by creating a common create method that avoids having to know how to initialize the common attributes of the class.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643858481
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
.setObjectType(queryCtx.objectType())
.setObjectName(queryCtx.objectName())
.setStartIndex(queryCtx.startIndex())
.setStopIndex(queryCtx.stopIndex())
.setFragment(queryCtx.fragment())
- .build())
+ .setSummary(queryCtx.summary())
+ .build()
+ } else {
Review Comment:
Is this reall an unconditional else?
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
.setObjectType(queryCtx.objectType())
.setObjectName(queryCtx.objectName())
.setStartIndex(queryCtx.startIndex())
.setStopIndex(queryCtx.stopIndex())
.setFragment(queryCtx.fragment())
- .build())
+ .setSummary(queryCtx.summary())
+ .build()
+ } else {
Review Comment:
Is this really an unconditional else?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642558434
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
~Yeah, I think we should do. Let me take a look separately after this PR. This PR isn't related to the original implementation.~
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643739251
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
I believe this is because the names are different in Python and Scala.
**Python**
https://github.com/apache/spark/blob/f0b7cfa56cb90ef70132d9656299956cbde00b53/python/pyspark/sql/column.py#L115
**Scala**
https://github.com/apache/spark/blob/f0b7cfa56cb90ef70132d9656299956cbde00b53/connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/Column.scala#L655
> Can we do the same thing in Python?
I think one way to do this is to add one `dict` into `with_origin` that can get the name of the Scala function corresponding to the Python function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643859355
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
Review Comment:
did we never set this before?
##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala:
##########
@@ -118,15 +118,27 @@ private[connect] object ErrorUtils extends Logging {
sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass)
}
for (queryCtx <- sparkThrowable.getQueryContext) {
- sparkThrowableBuilder.addQueryContexts(
- FetchErrorDetailsResponse.QueryContext
- .newBuilder()
+ val builder = FetchErrorDetailsResponse.QueryContext
+ .newBuilder()
+ val context = if (queryCtx.contextType() == QueryContextType.SQL) {
+ builder
+ .setContextType(FetchErrorDetailsResponse.QueryContext.ContextType.SQL)
.setObjectType(queryCtx.objectType())
.setObjectName(queryCtx.objectName())
.setStartIndex(queryCtx.startIndex())
.setStopIndex(queryCtx.stopIndex())
.setFragment(queryCtx.fragment())
- .build())
+ .setSummary(queryCtx.summary())
Review Comment:
same?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643992523
##########
python/pyspark/errors/exceptions/captured.py:
##########
@@ -379,17 +386,12 @@ class UnknownException(CapturedException, BaseUnknownException):
"""
-class QueryContext(BaseQueryContext):
+class SQLQueryContext(BaseQueryContext):
Review Comment:
Only parent class `QueryContext` is an API (at `pyspark.errors.QueryContext`) for now. This is at least consistent with Scala side.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641964734
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
My point is different. The callsite origin is something that is very generic and very useful for relations as well. That the origin today is only applicable to expressions is a side-effect of the implementation.
My goal is to have something like this
```
message Origin {
}
message Relation {
RelationCommon common = 1;
message RelationCommon {
Origin origin = 1;
}
}
message Expression {
ExpressionCommon common = 1;
message ExpressionCommon {
Origin origin = 1;
}
}
```
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1619590490
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -405,3 +407,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
+ // (Required) Indicate the origin type.
+ oneof function {
+ PythonOrigin python_origin = 1;
+ }
+}
+
+message PythonOrigin {
Review Comment:
I mean, I don't mind combining it for now if you believe it won't be language-specific.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642534389
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Yeah, I think we should do. This PR doesn't change the original implementation though. I will create a new PR after this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46789:
URL: https://github.com/apache/spark/pull/46789#issuecomment-2175508139
There are three followups to make:
- https://github.com/apache/spark/pull/46789#issuecomment-2175453126
- https://github.com/apache/spark/pull/46789#discussion_r1643779640
- SPARK-48639: Add origin like Expression.ExpressionCommon
Working on them this week.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #46789: [SPARK-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect
URL: https://github.com/apache/spark/pull/46789
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642534389
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Yeah, I think we should do. This PR doesn't change the original implementation though. I will create a new PR after this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643779640
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Actually we can leverage `pyspark.util.try_simplify_traceback` but let's do this in a separate PR.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641616617
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -405,3 +407,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
+ // (Required) Indicate the origin type.
+ oneof function {
+ PythonOrigin python_origin = 1;
+ }
+}
+
+message PythonOrigin {
Review Comment:
`StackTraceElement` has JDK dedicated method and fields (e.g., `classLoaderName`). I think we should have a dedicated one for individual languages.
While `DataFrameQueryContext` / `SQLQueryContext` have common information (for now), I think we will end up with having some language-specific and dedicated information for both APIs in the future.
However, I am open to having common one. There is a way to have the common (and, e.g., throw an exception if that information doesn't make sense in some languages).
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1641617771
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
The current implementation of tracking lazy evaluation is specific to `Expression`s actually (see `org.apache.spark.sql.withOrigin`.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642320042
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -342,6 +344,11 @@ message Expression {
}
}
+message ExpressionCommon {
Review Comment:
It's same as `Relation.RelationCommon` so it's more for consistency (so we can reuse `Origin` as well for call site). I think it's fine.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642396732
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Ah, this is actually a bug fix. Previously it was (rouhgly) checking the Scala side `DataFrameQueryContext.fragment` which had to be `DataFrameQueryContext.pysparkFragement`. Now it correctly checks `DataFrameQueryContext.pysparkFragement`.
This is because we now merge `pysparkFragment` to `fragment` at `DataFrameQueryContext`, and it correctly tests the fragment.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #46789:
URL: https://github.com/apache/spark/pull/46789#issuecomment-2137141381
cc @zhengruifeng @hvanhovell @itholic @cloud-fan @MaxGekk I need your look here 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642314889
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -342,6 +344,11 @@ message Expression {
}
}
+message ExpressionCommon {
Review Comment:
are we over-engineering? what else can be put here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643723445
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
cc @itholic
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643861005
##########
python/pyspark/errors/exceptions/captured.py:
##########
@@ -409,13 +411,34 @@ def fragment(self) -> str:
def callSite(self) -> str:
return str(self._q.callSite())
- def pysparkFragment(self) -> Optional[str]: # type: ignore[return]
- if self.contextType() == QueryContextType.DataFrame:
- return str(self._q.pysparkFragment())
+ def summary(self) -> str:
+ return str(self._q.summary())
+
+
+class DataFrameQueryContext(BaseQueryContext):
Review Comment:
Isn't the type annotation wrong here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643862057
##########
python/pyspark/errors/exceptions/captured.py:
##########
@@ -409,13 +411,34 @@ def fragment(self) -> str:
def callSite(self) -> str:
return str(self._q.callSite())
- def pysparkFragment(self) -> Optional[str]: # type: ignore[return]
- if self.contextType() == QueryContextType.DataFrame:
- return str(self._q.pysparkFragment())
+ def summary(self) -> str:
+ return str(self._q.summary())
+
+
+class DataFrameQueryContext(BaseQueryContext):
Review Comment:
I see this is for the classic side.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642053997
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
Ah sure.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1644013715
##########
python/pyspark/sql/connect/expressions.py:
##########
@@ -89,7 +90,16 @@ class Expression:
"""
def __init__(self) -> None:
- pass
+ origin = current_origin()
+ fragment = origin.fragment
+ call_site = origin.call_site
Review Comment:
I don't quite like this too actually. Yes, we can pass the explicit callsite around. It happened to be like this to match the implementation across Catalyst Optimizer, Scala, and PySpark w/ Spark Classic.
Let me revisit this to fix up Spark Classic side together and see if we can do.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1642562208
##########
python/pyspark/sql/tests/test_dataframe_query_context.py:
##########
@@ -41,7 +41,7 @@ def test_dataframe_query_context(self):
error_class="DIVIDE_BY_ZERO",
message_parameters={"config": '"spark.sql.ansi.enabled"'},
query_context_type=QueryContextType.DataFrame,
- pyspark_fragment="divide",
+ fragment="__truediv__",
Review Comment:
Ah, actually it points out correctly. `/` is a shortcut of `__truediv__`, and it points out the line number properly.
The missing part is to hide `pyspark.*`. I will take a separate look.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643866010
##########
python/pyspark/sql/connect/expressions.py:
##########
@@ -89,7 +90,16 @@ class Expression:
"""
def __init__(self) -> None:
- pass
+ origin = current_origin()
+ fragment = origin.fragment
+ call_site = origin.call_site
Review Comment:
I think this is what I don't like about this approach at least for Spark Connect. There is no need to use the thread-local for this propagation.
The outside wrapper manipulates the global state for the constructor to pick up here. Shouldn't we simply generate the origin here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1643860158
##########
python/pyspark/errors/exceptions/captured.py:
##########
@@ -379,17 +386,12 @@ class UnknownException(CapturedException, BaseUnknownException):
"""
-class QueryContext(BaseQueryContext):
+class SQLQueryContext(BaseQueryContext):
Review Comment:
Do we consider this a private / developer API?
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1618804356
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -405,3 +407,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
+ // (Required) Indicate the origin type.
+ oneof function {
+ PythonOrigin python_origin = 1;
+ }
+}
+
+message PythonOrigin {
Review Comment:
Please don;t name stuff Python/Scala if it is not language specific.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1640210979
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -54,6 +54,8 @@ message Expression {
google.protobuf.Any extension = 999;
}
+ // (Optional) Keep the information of the origin for this expression such as stacktrace.
+ Origin origin = 18;
Review Comment:
We already have something similar in `RelationCommon` called `source_info`.
If we do this PR we should mark the ID 1 in RelationCommon as reserved and remove the value. Then we should add it back as `Origin` with a new ID.
Now, for expressions, I think we should probably create a similar `ExpressionCommon` message and put it there.
--
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-48459][CONNECT][PYTHON] Implement DataFrameQueryContext in Spark Connect [spark]
Posted by "grundprinzip (via GitHub)" <gi...@apache.org>.
grundprinzip commented on code in PR #46789:
URL: https://github.com/apache/spark/pull/46789#discussion_r1640211247
##########
connector/connect/common/src/main/protobuf/spark/connect/expressions.proto:
##########
@@ -407,3 +409,18 @@ message NamedArgumentExpression {
// (Required) The value expression of the named argument.
Expression value = 2;
}
+
+message Origin {
Review Comment:
this needs to be moved to a common place as it's not specific to Expressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: 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