Support GetIndexedFieldExpr for ScalarValue #2196
Conversation
| )), | ||
| ColumnarValue::Scalar(scalar) => match (scalar.get_datatype(), &self.key) { | ||
| (DataType::List(v), ScalarValue::Int64(Some(i))) => { | ||
| let wrapper = scalar.to_array(); |
There was a problem hiding this comment.
Should I implement a new method that will convert Vec (which ScalarValue holds) for List instead of using the current API and temporarily ArrayRef? For example: as_array_list()?
Thanks
cC @alamb
There was a problem hiding this comment.
I wonder if initially you might be able to use ScalarValue::to_array_of_size to convert the scalar argument into an ArrayRef and then use the same code as above:
Another alternative might be to use the take kernel with something like
There was a problem hiding this comment.
to_array uses to_array_of_size under the hood, but I cannot reuse the code above, because it returns an ColumnarValue::Array, but in the case with ColumnarValue::Scalar we need to return ColumnarValue::Scalar.
I did another draft in af8c77f WDYT?
Thanks
There was a problem hiding this comment.
@alamb Sorry for the ping, just a friendly reminder that I am still waiting for advice on how to solve it. Thank you!
ovr
changed the title
|
|
||
| #[tokio::test] | ||
| async fn test_array_index() -> Result<()> { | ||
| test_expression!("([5,4,3,2,1])[1]", "5"); |
There was a problem hiding this comment.
https://www.postgresql.org/docs/current/arrays.html
I verified that the subscripts are 1 based 👍
The array subscript numbers are written within square brackets. By default PostgreSQL uses a one-based numbering convention for arrays, that is, an array of n elements starts with array[1] and ends with array[n].
datafusion/core/tests/sql/expr.rs
Outdated
| test_expression!("([5,4,3,2,1])[1]", "5"); | ||
| test_expression!("([5,4,3,2,1])[5]", "1"); | ||
| test_expression!("([5,4,3,2,1])[100]", "NULL"); | ||
| test_expression!("([5,4,3,2,1])[-1]", "NULL"); |
There was a problem hiding this comment.
I wonder if you want to potentially try nested lists. Something like
| test_expression!("([5,4,3,2,1])[-1]", "NULL"); | |
| test_expression!("([5,4,3,2,1])[-1]", "NULL"); | |
| test_expression!("([[123],[4,5,6]])[2]", "[4,5,6]"); |
There was a problem hiding this comment.
Right now, it's not possible to define multi-dimension arrays via SQL.
| ColumnarValue::Scalar(_) => Err(DataFusionError::NotImplemented( | ||
| "field access is not yet implemented for scalar values".to_string(), | ||
| )), | ||
| ColumnarValue::Scalar(scalar) => match (scalar.get_datatype(), &self.key) { |
There was a problem hiding this comment.
I am not sure I fully follow this code -- Since it is creating a ArrayRef from the ColumnarValue::Scalar, I wonder why it can't use the same code as the ColumnarValue::Array case and call to_arrow()?
So for example, rather than
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let arg = self.arg.evaluate(batch)?;
match arg {
ColumnarValue::Array(array) => match (array.data_type(), &self.key) {
...It could look like:
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
let array = self.arg.evaluate(batch)?
// convert to Arrayref
.to_arrow();
match (array.data_type(), &self.key) {
...That way the same code would be used for the array and scalar cases of ColumnarValue
github-actions
bot
added
core
| } else { | ||
| Ok(Field::new(&i.to_string(), lt.data_type().clone(), false)) | ||
| } | ||
| Ok(Field::new(&i.to_string(), lt.data_type().clone(), true)) |
There was a problem hiding this comment.
I've removed the check for negative integers to be compatible with PostgreSQL.
|
Rebased ✅ Changed to simplify/reuse logic across Scalar/ArrayRef. cC @alamb |
andygrove
changed the title
|
I'll go ahead and merge this later today unless @alamb has any additional feedback |
3 participants
Closes: #2207
Hello!
I've opened a PR as a Draft to indicate that I am working on the task of supporting the array index operator in DF.
Breaking changes
Thanks