Spark: Support read with settings #367

harryshi10 · 2024-11-08T06:25:32Z

Summary

allow read with settings.

close #272

CLAassistant · 2024-11-08T06:25:38Z

All committers have signed the CLA.

pan3793 · 2024-11-08T08:55:27Z

...-3.3/clickhouse-spark/src/main/scala/org/apache/spark/sql/clickhouse/ClickHouseSQLConf.scala

@@ -209,4 +209,13 @@ object ClickHouseSQLConf {
      .stringConf
      .transform(_.toLowerCase)
      .createWithDefault("binary")
+
+  val READ_WITH_SETTINGS: ConfigEntry[String] =
+    buildConf("spark.clickhouse.read.withSettings")


Suggested change

buildConf("spark.clickhouse.read.withSettings")

buildConf("spark.clickhouse.read.settings")

will also renaming the val to READ_SETTINGS

pan3793 · 2024-11-08T08:57:55Z

...-3.3/clickhouse-spark/src/main/scala/org/apache/spark/sql/clickhouse/ClickHouseSQLConf.scala

+
+  val READ_WITH_SETTINGS: ConfigEntry[String] =
+    buildConf("spark.clickhouse.read.withSettings")
+      .doc("Query-level settings when reading from ClickHouse. e.g. `final=1, max_execution_time=5`")


from the spark perspective, it's a session-level configuration

SET spark.clickhouse.read.settings=final=1; SELECT * FROM ch.db.t1; -- affected SELECT * FROM ch.db.t2; -- affected too

agree. will update the doc

pan3793 · 2024-11-08T09:00:14Z

spark-3.3/clickhouse-spark/src/main/scala/com/clickhouse/spark/read/ClickHouseReader.scala

@@ -60,6 +61,7 @@ abstract class ClickHouseReader[Record](
       |WHERE (${part.partFilterExpr}) AND (${scanJob.filtersExpr})
       |${scanJob.groupByClause.getOrElse("")}
       |${scanJob.limit.map(n => s"LIMIT $n").getOrElse("")}
+       |${if (readWithSettings.nonEmpty) s"SETTINGS $readWithSettings" else ""}


Suggested change

|${if (readWithSettings.nonEmpty) s"SETTINGS $readWithSettings" else ""}

|${readSettings.map(settings => s"SETTINGS $settings" else ""}

pan3793 · 2024-11-08T09:01:25Z

...-3.3/clickhouse-spark/src/main/scala/org/apache/spark/sql/clickhouse/ClickHouseSQLConf.scala

+      .version("0.9.0")
+      .stringConf
+      .transform(_.toLowerCase)
+      .createWithDefault("")


Suggested change

.createWithDefault("")

.createOptional

will update it to OptionalConfigEntry accordingly

mzitnik

@harryshi10 Can we provide tests of the new feature?

mzitnik · 2024-11-10T08:04:39Z

@pan3793, do you think we also need to provide write.settings?

pan3793 · 2024-11-11T02:08:06Z

...-3.3/clickhouse-spark/src/main/scala/org/apache/spark/sql/clickhouse/ClickHouseSQLConf.scala

+      .doc("Settings when read from ClickHouse. e.g. `final=1, max_execution_time=5`")
+      .version("0.9.0")
+      .stringConf
+      .transform(_.toLowerCase)


why should convert the input to lower case?

in case we'd like to log scanQuery which keeps SQL keywords in uppercase and other text in lowercase

pan3793 · 2024-11-11T02:09:05Z

code change lgtm, would be great if you could provide a test case

pan3793 · 2024-11-11T03:17:27Z

@pan3793, do you think we also need to provide write.settings?

yes, it could be implemented in another PR.

Additionally, SPARK-36680 (Spark 4.0) provides a more intuitive SQL syntax for this case

SELECT * FROM $t1 WITH (split-size = 5)

harryshi10 · 2024-11-12T21:40:16Z

sorry I'm still a rookie at Scala. but I will try to write a UT for this new feature

mzitnik · 2024-11-14T11:35:10Z

@harryshi10 could please sign CLA

harryshi10 · 2024-11-15T17:27:28Z

@harryshi10 could please sign CLA

done

harryshi10 · 2024-11-22T06:54:41Z

code change lgtm, would be great if you could provide a test case

Sorry, I can’t provide a unit test, but here’s a test case I ran locally with PySpark.

env - ClickHouse = 24.10.2.80, Spark = 3.5.0

A SummingMergeTree with two records sharing the same key shows duplicates when queried without FINAL, but returns aggregated results when queried with FINAL.

In Spark, setting final=0 or final=1 in spark.clickhouse.read.settings controls whether the results are aggregated or not, with final=0 showing non-aggregated results and final=1 providing aggregated results.

I also tested that adding final=0 or 1 in spark.clickhouse.read.settings has no side effect on other engines, such as MergeTree.

pan3793 · 2024-12-03T10:28:26Z

please refer to the error message to fix the docs, other lgtm

harryshi10 · 2024-12-08T06:38:59Z

please refer to the error message to fix the docs, other lgtm

doc updated to indicate the default value is None. sorry for missing of that and the delay of reply

mzitnik

Have we changed something in our tests?

mzitnik · 2024-12-15T08:32:22Z

@harryshi10 can you please sign the CLA

huashi-st · 2024-12-16T05:24:16Z

@harryshi10 can you please sign the CLA

Signed. Apologies, I just realized I used two different accounts to push this PR and signed with both.

allow read with settings

ccd6d3f

pan3793 reviewed Nov 8, 2024

View reviewed changes

huashi-st added 2 commits November 8, 2024 18:11

make optional

1a7ea06

update doc

db3cd5a

mshustov requested review from mzitnik and pan3793 November 9, 2024 13:13

mzitnik reviewed Nov 10, 2024

View reviewed changes

pan3793 changed the title ~~feature/allow-read-with-settings~~ Spark: Support read with settings Nov 11, 2024

pan3793 reviewed Nov 11, 2024

View reviewed changes

mshustov mentioned this pull request Nov 18, 2024

Apache Spark - Docs refactoring ClickHouse/clickhouse-docs#2789

Merged

mshustov requested a review from mzitnik November 18, 2024 11:31

mshustov requested a review from pan3793 December 3, 2024 09:01

update doc to describe default value is None

ddd62fd

mzitnik approved these changes Dec 8, 2024

View reviewed changes

mzitnik reviewed Dec 8, 2024

View reviewed changes

mzitnik merged commit 7a06a13 into ClickHouse:main Dec 9, 2024
16 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark: Support read with settings #367

Spark: Support read with settings #367

harryshi10 commented Nov 8, 2024 •

edited by pan3793

Loading

CLAassistant commented Nov 8, 2024 •

edited

Loading

pan3793 Nov 8, 2024

harryshi10 Nov 8, 2024

harryshi10 Nov 9, 2024

pan3793 Nov 8, 2024

harryshi10 Nov 8, 2024

harryshi10 Nov 9, 2024

pan3793 Nov 8, 2024

harryshi10 Nov 9, 2024

pan3793 Nov 8, 2024

harryshi10 Nov 9, 2024

harryshi10 Nov 9, 2024

mzitnik left a comment •

edited

Loading

mzitnik commented Nov 10, 2024

pan3793 Nov 11, 2024

harryshi10 Nov 11, 2024 •

edited

Loading

pan3793 commented Nov 11, 2024

pan3793 commented Nov 11, 2024

harryshi10 commented Nov 12, 2024

mzitnik commented Nov 14, 2024

harryshi10 commented Nov 15, 2024

harryshi10 commented Nov 22, 2024

pan3793 commented Dec 3, 2024

harryshi10 commented Dec 8, 2024

mzitnik left a comment

mzitnik commented Dec 15, 2024

huashi-st commented Dec 16, 2024

	buildConf("spark.clickhouse.read.withSettings")
	buildConf("spark.clickhouse.read.settings")

	\|${if (readWithSettings.nonEmpty) s"SETTINGS $readWithSettings" else ""}
	\|${readSettings.map(settings => s"SETTINGS $settings" else ""}

Spark: Support read with settings #367

Spark: Support read with settings #367

Conversation

harryshi10 commented Nov 8, 2024 • edited by pan3793 Loading

Summary

CLAassistant commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzitnik left a comment • edited Loading

Choose a reason for hiding this comment

mzitnik commented Nov 10, 2024

Choose a reason for hiding this comment

harryshi10 Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

pan3793 commented Nov 11, 2024

pan3793 commented Nov 11, 2024

harryshi10 commented Nov 12, 2024

mzitnik commented Nov 14, 2024

harryshi10 commented Nov 15, 2024

harryshi10 commented Nov 22, 2024

pan3793 commented Dec 3, 2024

harryshi10 commented Dec 8, 2024

mzitnik left a comment

Choose a reason for hiding this comment

mzitnik commented Dec 15, 2024

huashi-st commented Dec 16, 2024

harryshi10 commented Nov 8, 2024 •

edited by pan3793

Loading

CLAassistant commented Nov 8, 2024 •

edited

Loading

mzitnik left a comment •

edited

Loading

harryshi10 Nov 11, 2024 •

edited

Loading