You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hvanhovell (via GitHub)" <gi...@apache.org> on 2024/09/06 14:03:12 UTC
[PR] [SPARK-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
hvanhovell opened a new pull request, #48017:
URL: https://github.com/apache/spark/pull/48017
### What changes were proposed in this pull request?
This PR moves a number of streaming classes into sql/api. This is the initial step in create a shared interface for streaming operations.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing 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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #48017:
URL: https://github.com/apache/spark/pull/48017#discussion_r1750772454
##########
sql/api/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListener.scala:
##########
Review Comment:
@WweiL @bogao007 can you take a look.
TL;DR of the changes here. It turns out that by adding the event super class (which was previously not in connect), serialization with Json4s and deserialization with Jackson is no longer compatible. Jackson complains that the `Event` field is missing. For now I have fixed this by manually parsing the Json.
There are a couple of alternatives:
- Use Jackson for serialization as well. This will change the format a bit, but I think this is fine since this is unreleased. In this case we should also modify the protocol a bit, and drop the event name.
- Use proto instead of json. I am not sure what the logic behind using json is, but it has always struck me as weird that we use proto and some json.
--
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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #48017:
URL: https://github.com/apache/spark/pull/48017#discussion_r1750777801
##########
sql/api/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListener.scala:
##########
@@ -122,25 +171,22 @@ object StreamingQueryListener extends Serializable {
}
private[spark] object QueryStartedEvent {
- private val mapper = {
- val ret = new ObjectMapper() with ClassTagExtensions
- ret.registerModule(DefaultScalaModule)
- ret.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
- ret
- }
- private[spark] def jsonString(event: QueryStartedEvent): String =
Review Comment:
Remove this and the others because they were not used, and they led to some confusion on my end.
--
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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #48017:
URL: https://github.com/apache/spark/pull/48017#issuecomment-2335005209
This has an actual test failure. Investigating.
--
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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #48017:
URL: https://github.com/apache/spark/pull/48017#discussion_r1750777801
##########
sql/api/src/main/scala/org/apache/spark/sql/streaming/StreamingQueryListener.scala:
##########
@@ -122,25 +171,22 @@ object StreamingQueryListener extends Serializable {
}
private[spark] object QueryStartedEvent {
- private val mapper = {
- val ret = new ObjectMapper() with ClassTagExtensions
- ret.registerModule(DefaultScalaModule)
- ret.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
- ret
- }
- private[spark] def jsonString(event: QueryStartedEvent): String =
Review Comment:
Removed this and the others because they were not used, and they led to some confusion on my end.
--
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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on PR #48017:
URL: https://github.com/apache/spark/pull/48017#issuecomment-2339342017
Merging 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-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api [spark]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #48017: [SPARK-49287][SPARK-49432][CONNECT][SQL] Move streaming classes to sql/api
URL: https://github.com/apache/spark/pull/48017
--
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