feat(java): support set options for spark datasource api #3366
Conversation
| System.arraycopy(namespace, 0, this.namespace, 0, namespace.length); | ||
| this.namespace[namespace.length] = SEPARATOR; | ||
| int i = namespace.length + 1; | ||
| for (Map.Entry<String, String> entry : options.entrySet()) { |
There was a problem hiding this comment.
As the iceberg also need to put the options in path, the LanceIdentifier put he option in the namespace.
https://github.com/apache/iceberg/blob/fc923b3af65b0e3cb28a9afb69f7fd05c88f62ca/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java#L129
There was a problem hiding this comment.
If the following test can be supported. It will save my day. we can read lance with pure spark sql.
@Test
void versionInSQL() {
String uri = LanceConfig.getDatasetUri(dbPath, TestUtils.TestTable1Config.datasetName);
uri += "#####version=1";
String sql = "SELECT * FROM lance.`" + uri + "`";
Dataset<Row> df = spark.sql(sql);
assertEquals(2, df.count());
}There was a problem hiding this comment.
Is this common use of spark data source？
There was a problem hiding this comment.
I don't know. but I think this is reasonable. I also want to listen other's advice.
| * dataset URI and the namespace. The namespace is an array of strings, which contains the namespace | ||
| * of the dataset and the options. The options are key-value pairs, which are separated by "#####". |
There was a problem hiding this comment.
What is the purpose of the namespace? Why does it store a serialized copy of the options?
There was a problem hiding this comment.
The namespace was stand for catalog and database name. In this case, the catalog load table and create table will convert Lance Identifier into IdentifierImpl which only have namespaces and name. The way Iceberg does is add options in the path stored in name. I store the options in namespaces.
There was a problem hiding this comment.
Maybe we can wait until the design of Lance Catalog landing?
There was a problem hiding this comment.
The datasource api is not depend on catalog.
The AI user maybe do not use the catalog.
There was a problem hiding this comment.
Ok, I checked this file has existed before, not introduced recently. Will review it later.
| public class LanceIdentifier implements Identifier { | ||
| private final String[] namespace = new String[] {"default"}; | ||
| public static final String SEPARATOR = "#####"; |
There was a problem hiding this comment.
Is #### a customary separator or delimiter in spark ecosystem? We may need some comments or remarks to describe its purpose?
| .format("lance") | ||
| .option("version", "1") | ||
| .load(LanceConfig.getDatasetUri(dbPath, TestUtils.TestTable1Config.datasetName)); | ||
| assertEquals(2, df.count()); |
There was a problem hiding this comment.
Can we add some description of why there is 2 to make the assertion more readable? The first version, wrote 2 records?
|
Maybe it's not a good way to implement this case. Mark it draft first |
SaintBacchus
force-pushed
the
LanceSparkId
branch
from
6df2f49 to
78ea88a
Compare
|
@yanghua @wjones127 @chenkovsky please review it again now. I put the read and write options in the lance uri and removing from the namespaces as the Iceberg does. |
use
optionto set options for lance spark datasource like readversion:Why do it in this way
The spark data source API will change the
LanceIdentifierintoIdentifierImplandIdentifierImplonly have namespaces and name without the options in the data source.How to do that
Inspired by Iceberg putting the options in the name. I designed the name
LanceIdentifierwith the formatname#key1=value1&key2=value2. These options will only set the read and write options. The storage options should be set in spark configuration and not in option since the AK/SK format is complex.