You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/11/17 19:17:33 UTC
[GitHub] [iceberg] samarthjain opened a new pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
samarthjain opened a new pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-729402986
What is the use case of enabling/disabling vectorization per session?
If we decide to have session configs, should we have a proper place for them like our own SQL config?
Shall we be using `SessionConfigSupport` in Spark for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r526527006
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +157,15 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ Option<String> option = SparkSession.active().sparkContext().getConf()
+ .getOption("spark.iceberg.vectorization.enabled");
Review comment:
You should be able to use `SparkSession.active().conf().get("key", null)` to get the value or null. No need to use the Spark context's conf.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r525623747
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +156,13 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ boolean batchReadsSparkSessionConf = SparkSession.active().sparkContext().getConf()
+ .getBoolean("read.parquet.vectorization.enabled", true);
Review comment:
Don't Spark settings need to start with `spark.`? Otherwise they are discarded.
How about "spark.iceberg.vectorization.enabled`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r525623524
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +156,13 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ boolean batchReadsSparkSessionConf = SparkSession.active().sparkContext().getConf()
+ .getBoolean("read.parquet.vectorization.enabled", true);
+ boolean batchReadsEnabledTableProp = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
+ PropertyUtil.propertyAsBoolean(table.properties(),
+ TableProperties.PARQUET_VECTORIZATION_ENABLED,
+ TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
Review comment:
Looks like these 4 lines didn't actually change. Can you revert the whitespace changes 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samarthjain commented on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
samarthjain commented on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-730050443
> Shall we be using `SessionConfigSupport` in Spark for this?
@aokolnychyi - we have run into a couple of use cases internally where users have run into issues using vectorized reads on their iceberg tables. However, the only way one can currently disable vectorized reads is by changing the table property. Since a table can be read by multiple jobs/users, disabling vectorized reads at a table property level prevents all users/jobs from using vectorized reads. By having a session level control instead, jobs/users can decide themselves instead of "globally" restricting everyone.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r525623747
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +156,13 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ boolean batchReadsSparkSessionConf = SparkSession.active().sparkContext().getConf()
+ .getBoolean("read.parquet.vectorization.enabled", true);
Review comment:
Don't Spark settings need to start with `spark.`? Otherwise they are discarded.
How about `spark.iceberg.vectorization.enabled`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi edited a comment on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-729402986
What is the use case of enabling/disabling vectorization per session? Do you guys find that useful internally?
If we decide to have session configs, should we have a proper place for them like our own SQL config?
Shall we be using `SessionConfigSupport` in Spark for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-730059373
> Shall we be using SessionConfigSupport in Spark for this?
Does `SessionConfigSupport` work with catalogs? I thought it was only supported in some `DataSource` paths. I typically use catalog configuration, which I think cleanly removes the need for that interface. That may be a clean way to plug in here, I don't really have a preference.
@samarthjain, I think you probably want to update the Spark 3 path as well, since that's what we use in 2.4.4.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-731271434
You are right `SessionConfigSupport` is for data sources only.
One more question: we have other sessions configs like "spark.sql.iceberg.check-nullability" under `sql` namespace. Do we want to match?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r525628233
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +156,13 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ boolean batchReadsSparkSessionConf = SparkSession.active().sparkContext().getConf()
+ .getBoolean("read.parquet.vectorization.enabled", true);
+ boolean batchReadsEnabledTableProp = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
+ PropertyUtil.propertyAsBoolean(table.properties(),
+ TableProperties.PARQUET_VECTORIZATION_ENABLED,
+ TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ this.batchReadsEnabled = !batchReadsSparkSessionConf ? false : batchReadsEnabledTableProp;
Review comment:
Looks like the logic here is too complicated. I think what you're trying to do is disable vectorized reads if the session conf disables them, and otherwise delegate to the table property. In other words, the session conf and the table property must be enabled to use vectorization. If that's the case, then this could be `(batchReadsSparkSessionConf && batchReadsEnabledTableProp)`.
I'm not sure that logic is a good idea, either. This may have surprising behavior because a user may expect that `true` enables vectorization for all tables when used as a session property.
I think a better way to configure is to choose an order of priority and delegate to the next config if the option is not set. Here, I think session should override table. If session is set, return whatever it is. Otherwise, use the table property.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r526526624
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +157,15 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ Option<String> option = SparkSession.active().sparkContext().getConf()
+ .getOption("spark.iceberg.vectorization.enabled");
+ if (option.isDefined()) {
+ this.batchReadsEnabled = Boolean.valueOf(option.get());
+ } else {
+ this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
+ PropertyUtil.propertyAsBoolean(table.properties(), TableProperties.PARQUET_VECTORIZATION_ENABLED,
+ TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
Review comment:
Continuation indents are 4 spaces or 2 indents from the previous. Could you update 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi edited a comment on pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
aokolnychyi edited a comment on pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#issuecomment-729402986
What is the use case of enabling/disabling vectorization per session? Do you guys find that useful?
If we decide to have session configs, should we have a proper place for them like our own SQL config?
Shall we be using `SessionConfigSupport` in Spark for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] samarthjain commented on a change in pull request #1778: Parquet - Add a spark session configuration for controlling enabling vectorized reads
Posted by GitBox <gi...@apache.org>.
samarthjain commented on a change in pull request #1778:
URL: https://github.com/apache/iceberg/pull/1778#discussion_r525627285
##########
File path: spark2/src/main/java/org/apache/iceberg/spark/source/Reader.java
##########
@@ -156,10 +156,13 @@
this.io = io;
this.encryptionManager = encryptionManager;
this.caseSensitive = caseSensitive;
-
- this.batchReadsEnabled = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
- PropertyUtil.propertyAsBoolean(table.properties(),
- TableProperties.PARQUET_VECTORIZATION_ENABLED, TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
+ boolean batchReadsSparkSessionConf = SparkSession.active().sparkContext().getConf()
+ .getBoolean("read.parquet.vectorization.enabled", true);
+ boolean batchReadsEnabledTableProp = options.get("vectorization-enabled").map(Boolean::parseBoolean).orElseGet(() ->
+ PropertyUtil.propertyAsBoolean(table.properties(),
+ TableProperties.PARQUET_VECTORIZATION_ENABLED,
+ TableProperties.PARQUET_VECTORIZATION_ENABLED_DEFAULT));
Review comment:
The variable name was changed which caused the previous line to exceed the 120 character limit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org