HyukjinKwon commented on code in PR #43352: URL: https://github.com/apache/spark/pull/43352#discussion_r1427431997
########## connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala: ########## @@ -115,6 +115,17 @@ private[connect] object ErrorUtils extends Logging { if (sparkThrowable.getErrorClass != null) { sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass) } + for (queryCtx <- sparkThrowable.getQueryContext) { + sparkThrowableBuilder.addQueryContexts( + FetchErrorDetailsResponse.QueryContext + .newBuilder() + .setObjectType(queryCtx.objectType()) + .setObjectName(queryCtx.objectName()) + .setStartIndex(queryCtx.startIndex()) Review Comment: Alright, there are two issues. TL;DR 1. There are many places that does not provide `Origin` that contains `QueryContext` at `QueryCompilationErrors`. So the `QueryContext` is often missing (e.g., the case above). cc @MaxGekk 2. `withOrigin` is not being invoked, and the stacktrace is undefined in Spark Connect server. So always we set `SQLQueryContext`. That's why this specific code did not throw an exception from https://github.com/apache/spark/pull/43334/files#diff-b3bc05fec45cd951053b2876c71c7730b63789cb4336a7537a6654c724db3241R586-R589 because it has never been `DataFrameContext`. We should invoke `withOrigin` in Spark Connect server. cc @heyihong @juliuszsompolski FYI. 3. The current logic in `ErrorUtils.scala` should handle the case of `DataFrameQueryContext` e.g., `DataFrameQueryContext.stopIndex()` will throw an exception. Should we: - Set a default value in `DataFrameQueryContext.stopIndex()` instead of throwing an exception? @MaxGekk - Or, make the protobuf message for this optional, and throw an exception from Spark Connect client sides? @heyihong @juliuszsompolski ########## connector/connect/server/src/main/scala/org/apache/spark/sql/connect/utils/ErrorUtils.scala: ########## @@ -115,6 +115,17 @@ private[connect] object ErrorUtils extends Logging { if (sparkThrowable.getErrorClass != null) { sparkThrowableBuilder.setErrorClass(sparkThrowable.getErrorClass) } + for (queryCtx <- sparkThrowable.getQueryContext) { + sparkThrowableBuilder.addQueryContexts( + FetchErrorDetailsResponse.QueryContext + .newBuilder() + .setObjectType(queryCtx.objectType()) + .setObjectName(queryCtx.objectName()) + .setStartIndex(queryCtx.startIndex()) Review Comment: Alright, there are two issues. TL;DR 1. There are many places that does not provide `Origin` that contains `QueryContext` at `QueryCompilationErrors`. So the `QueryContext` is often missing (e.g., the case above). cc @MaxGekk 2. `withOrigin` is not being invoked, and the stacktrace is undefined in Spark Connect server. So always we set `SQLQueryContext`. That's why this specific code did not throw an exception from https://github.com/apache/spark/pull/43334/files#diff-b3bc05fec45cd951053b2876c71c7730b63789cb4336a7537a6654c724db3241R586-R589 because it has never been `DataFrameContext`. We should invoke `withOrigin` in Spark Connect server. cc @heyihong @juliuszsompolski FYI. 3. The current logic in `ErrorUtils.scala` should handle the case of `DataFrameQueryContext` e.g., `DataFrameQueryContext.stopIndex()` will throw an exception. Should we: - Set a default value in `DataFrameQueryContext.stopIndex()` instead of throwing an exception? @MaxGekk - Or, make the protobuf message for this optional, and throw an exception from Spark Connect client sides? @heyihong @juliuszsompolski -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org