feat(java): expose sql api for java api#4328
Conversation
|
@yanghua Please review this pr |
| } | ||
| } | ||
|
|
||
| public SqlQuery sql(String sql, String tableName) { |
There was a problem hiding this comment.
In rust, we make the tableName an optional arg. If users do not specify, the default name is dataset.
There was a problem hiding this comment.
In rust, we make the
tableNamean optional arg. If users do not specify, the default name isdataset.
If the name of the Dataset is custom and the tableName is not specified, the query will report an error. Therefore, I think it is necessary to enforce the specification of the correct tableName in terms of the interface's semantics to avoid misunderstandings by users.
There was a problem hiding this comment.
I think this would be challenged if rust or python doesn't have a table name parameter.
There was a problem hiding this comment.
tableName has been changed to Optional in order to keep consistency with python and rust.
| reader.close(); | ||
|
|
||
| // Test agg query | ||
| reader = dataset.sql("select sum(id) from " + NAME, NAME).intoBatchRecords(); |
There was a problem hiding this comment.
Can we also verify the result of the sum operation?
|
@fangbo, thanks for your contribution! Left some comments. |
| private boolean withRowId = false; | ||
| private boolean withRowAddr = false; | ||
|
|
||
| public SqlQuery(BufferAllocator allocator, Dataset dataset, String sql, String table) { |
There was a problem hiding this comment.
I think we may reuse allocator in the dataset? The issue here is who could be responsable for releasing this allocator
There was a problem hiding this comment.
The allocator is reused from Dataset's allocator.
There was a problem hiding this comment.
I mean this parameter of allocator may be unnecessary and expose some risk. We could directly get it from dataset.
There was a problem hiding this comment.
Does it mean that Dataset provides a new method like getAllocator() which returns the Allocator, and SqlQuery uses getAllocator() to get the shared Allocator ?
There was a problem hiding this comment.
Ye, that should be an option.
I think right now you could just use dataset.allocator if you put that code in the same package. There has been use cases like this.
There was a problem hiding this comment.
Good suggestion. The SqlQuery has been move to package com.lancedb.lance and it uses dataset.allocator.
| } | ||
| } | ||
|
|
||
| public SqlQuery sql(String sql, String tableName) { |
There was a problem hiding this comment.
I think this would be challenged if rust or python doesn't have a table name parameter.
yanghua
left a comment
yanghua
left a comment
There was a problem hiding this comment.
LGTM, if we can add a comment for the public api. It would be better.
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
looks good to me, +1 for adding some comments since this is public API.
4 participants
Add sql query for java api.
epic: #3950
See:
#4267
#4086