You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "przemekd (via GitHub)" <gi...@apache.org> on 2023/11/10 20:58:47 UTC
[PR] Core: lazily create locationProvider in SerializableTable [iceberg]
przemekd opened a new pull request, #9029:
URL: https://github.com/apache/iceberg/pull/9029
Closes #9025
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "przemekd (via GitHub)" <gi...@apache.org>.
przemekd commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1807918280
@aokolnychyi Sure. This one is not about serialization. The idea behind this fix is to allow to query (read) tables in Spark (and potentially other query engines using `SerializableTable`) that use the custom `LocationProvider`, but with not having to provide an implementation of this provider to the job. Right now all jobs reading from such tables need to provide the implementation which is troublesome and not necessary.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "przemekd (via GitHub)" <gi...@apache.org>.
przemekd commented on code in PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#discussion_r1401297380
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -237,7 +236,14 @@ public EncryptionManager encryption() {
@Override
public LocationProvider locationProvider() {
- return locationProvider;
+ if (lazyLocationProvider == null) {
+ synchronized (this) {
+ if (lazyLocationProvider == null) {
+ lazyLocationProvider = LocationProviders.locationsFor(location, properties);
Review Comment:
Makes sense. Change applied 👍🏻
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -237,7 +236,14 @@ public EncryptionManager encryption() {
@Override
public LocationProvider locationProvider() {
- return locationProvider;
+ if (lazyLocationProvider == null) {
+ synchronized (this) {
+ if (lazyLocationProvider == null) {
+ lazyLocationProvider = LocationProviders.locationsFor(location, properties);
Review Comment:
Makes sense. Change applied 👍🏻
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #9029:
URL: https://github.com/apache/iceberg/pull/9029
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1821791818
You are right, the data analyst doesn't have to create new locations in that case. I am convinced now.
Let me review the change itself.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "przemekd (via GitHub)" <gi...@apache.org>.
przemekd commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1820779641
@aokolnychyi We assume the data analyst uses a completely separate cluster, JAR is not populated on executors and not supplied with the job. With the fix provided she still can do her job fine, as she only needs to read records from the table.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1813526621
I am not sure I entirely understood how `SerializableTable` instances are created in your case. Is it created from the primary table before sending the table info to remote nodes?
Also, what do you mean by "not having to provide an 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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "przemekd (via GitHub)" <gi...@apache.org>.
przemekd commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1814274741
@aokolnychyi Sure. So, the problem is shortly described in the linked issue: https://github.com/apache/iceberg/issues/9025
I am not directly creating `SerializableTable` instance in my code, this is what Spark does (and most likely some other query engines) when reading an Iceberg table. Let me explain the problem in a detailed way.
Scenario:
1. Let's assume that there is a new implementation of a custom location provider `CustomLocationProvider` packaged into a custom-provider.jar package.
1. There is a Spark job that creates a new Iceberg table `iceberg_table_with_custom_location_provider` with custom location provider by setting `write.location-provider.impl` property on that table. The job provides an implementation of this provider (by using `--jars custom-provider.jar` option) and successfully writes new records into that table.
2. Now a data analyst wants to query this table. She starts `spark-sql` and tries to execute a query `SELECT * FROM iceberg_table_with_custom_location_provider LIMIT 10`. She does not provide the implementation of location provider that was set and used for this table. She is unable to get query results because of the error:
```
java.lang.IllegalArgumentException: Unable to find a constructor for implementation org.iceberg.custom.LocationProviders$DefaultLocationProvider of interface org.apache.iceberg.io.LocationProvider. Make sure the implementation is in classpath, and that it either has a public no-arg constructor or a two-arg constructor taking in the string base table location and its property string map.
at org.apache.iceberg.LocationProviders.locationsFor(LocationProviders.java:52)
at org.apache.iceberg.BaseMetastoreTableOperations.locationProvider(BaseMetastoreTableOperations.java:243)
at org.apache.iceberg.BaseTable.locationProvider(BaseTable.java:245)
at org.apache.iceberg.SerializableTable.<init>(SerializableTable.java:84)
at org.apache.iceberg.spark.source.SerializableTableWithSize.<init>(SerializableTableWithSize.java:36)
at org.apache.iceberg.spark.source.SerializableTableWithSize.copyOf(SerializableTableWithSize.java:48)
at org.apache.iceberg.spark.source.SparkBatch.planInputPartitions(SparkBatch.java:77)
at org.apache.spark.sql.execution.datasources.v2.BatchScanExec.inputPartitions$lzycompute(BatchScanExec.scala:63)
```
The problem is that Spark needs to create a `SerializableTable` object for getting input partitions in order to read data and it cannot create it without a location provider implementation provided. It is because location provider field is eagerly populated, event if that's not needed.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1820198393
Does the data analyst connect to a remote already running cluster? Do we assume the jar is populated on executors but not supplied by the analyst in the local shell?
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#discussion_r1401282710
##########
core/src/main/java/org/apache/iceberg/SerializableTable.java:
##########
@@ -237,7 +236,14 @@ public EncryptionManager encryption() {
@Override
public LocationProvider locationProvider() {
- return locationProvider;
+ if (lazyLocationProvider == null) {
+ synchronized (this) {
+ if (lazyLocationProvider == null) {
+ lazyLocationProvider = LocationProviders.locationsFor(location, properties);
Review Comment:
Minor: `lazyLocationProvider = ...` -> `this.lazyLocationProvider = ...` to match the existing style of the class. We usually use `this.` prefix when assigning a value to an instance variable to highlight that it is an instance variable. We don't use `this.` whenever accessing such variables so no need to change that.
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1822113350
Thanks for your patience, @przemekd!
--
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: issues-unsubscribe@iceberg.apache.org
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
Re: [PR] Core: lazily create locationProvider in SerializableTable [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #9029:
URL: https://github.com/apache/iceberg/pull/9029#issuecomment-1806884352
@przemekd, could you elaborate a bit on the custom `LocationProvider`? All implementations must be `Serializable`. Are you seeing issues with Java or Kryo serialization?
--
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: issues-unsubscribe@iceberg.apache.org
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