You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/08/07 19:04:57 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
alamb opened a new pull request, #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220
## Which issue does this PR close?
Related to https://github.com/apache/arrow-datafusion/issues/7193
Follows https://github.com/apache/arrow-datafusion/pull/7215 from @izveigor
## Rationale for this change
In order to have a good experience accessing fields, it should be easy to use the code that manipulates them
## What changes are included in this PR?
1. Rename `GetFieldAccessCharacteristic` to `GetFieldAccessSchema` to better reflect what it does
2. move the calculation into `GetFieldAccessSchema::get_accessed_field`
3. Reduce duplication
## Are these changes tested?
Covered by existing tests
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
4. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
No, these are unreleased APIs (added in https://github.com/apache/arrow-datafusion/pull/7215)
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220#discussion_r1286712027
##########
datafusion/expr/src/field_util.rs:
##########
@@ -20,64 +20,65 @@
use arrow::datatypes::{DataType, Field};
use datafusion_common::{plan_err, DataFusionError, Result, ScalarValue};
-pub enum GetFieldAccessCharacteristic {
- /// returns the field `struct[field]`. For example `struct["name"]`
+/// Types of the field access expression of a nested type, such as `Field` or `List`
+pub enum GetFieldAccessSchema {
+ /// Named field, For example `struct["name"]`
NamedStructField { name: ScalarValue },
- /// single list index
- // list[i]
+ /// Single list index, for example: `list[i]`
ListIndex { key_dt: DataType },
- /// list range `list[i:j]`
+ /// List range, for example `list[i:j]`
ListRange {
start_dt: DataType,
stop_dt: DataType,
},
}
-/// Returns the field access indexed by `key` and/or `extra_key` from a [`DataType::List`] or [`DataType::Struct`]
-/// # Error
-/// Errors if
-/// * the `data_type` is not a Struct or a List,
-/// * the `data_type` of extra key does not match with `data_type` of key
-/// * there is no field key is not of the required index type
-pub fn get_indexed_field(
- data_type: &DataType,
- field_characteristic: &GetFieldAccessCharacteristic,
-) -> Result<Field> {
- match field_characteristic {
- GetFieldAccessCharacteristic::NamedStructField{ name } => {
- match (data_type, name) {
- (DataType::Struct(fields), ScalarValue::Utf8(Some(s))) => {
- if s.is_empty() {
- plan_err!(
- "Struct based indexed access requires a non empty string"
- )
- } else {
- let field = fields.iter().find(|f| f.name() == s);
- field.ok_or(DataFusionError::Plan(format!("Field {s} not found in struct"))).map(|f| f.as_ref().clone())
+impl GetFieldAccessSchema {
+ /// Returns the schema [`Field`] from a [`DataType::List`] or
+ /// [`DataType::Struct`] indexed by this structure
+ ///
+ /// # Error
+ /// Errors if
+ /// * the `data_type` is not a Struct or a List,
+ /// * the `data_type` of extra key does not match with `data_type` of key
Review Comment:
It is better to replace `extra_key` and `key` with new terms
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220#discussion_r1287054931
##########
datafusion/expr/src/field_util.rs:
##########
@@ -20,64 +20,65 @@
use arrow::datatypes::{DataType, Field};
use datafusion_common::{plan_err, DataFusionError, Result, ScalarValue};
-pub enum GetFieldAccessCharacteristic {
- /// returns the field `struct[field]`. For example `struct["name"]`
+/// Types of the field access expression of a nested type, such as `Field` or `List`
+pub enum GetFieldAccessSchema {
+ /// Named field, For example `struct["name"]`
NamedStructField { name: ScalarValue },
- /// single list index
- // list[i]
+ /// Single list index, for example: `list[i]`
ListIndex { key_dt: DataType },
- /// list range `list[i:j]`
+ /// List range, for example `list[i:j]`
ListRange {
start_dt: DataType,
stop_dt: DataType,
},
}
-/// Returns the field access indexed by `key` and/or `extra_key` from a [`DataType::List`] or [`DataType::Struct`]
-/// # Error
-/// Errors if
-/// * the `data_type` is not a Struct or a List,
-/// * the `data_type` of extra key does not match with `data_type` of key
-/// * there is no field key is not of the required index type
-pub fn get_indexed_field(
- data_type: &DataType,
- field_characteristic: &GetFieldAccessCharacteristic,
-) -> Result<Field> {
- match field_characteristic {
- GetFieldAccessCharacteristic::NamedStructField{ name } => {
- match (data_type, name) {
- (DataType::Struct(fields), ScalarValue::Utf8(Some(s))) => {
- if s.is_empty() {
- plan_err!(
- "Struct based indexed access requires a non empty string"
- )
- } else {
- let field = fields.iter().find(|f| f.name() == s);
- field.ok_or(DataFusionError::Plan(format!("Field {s} not found in struct"))).map(|f| f.as_ref().clone())
+impl GetFieldAccessSchema {
+ /// Returns the schema [`Field`] from a [`DataType::List`] or
+ /// [`DataType::Struct`] indexed by this structure
+ ///
+ /// # Error
+ /// Errors if
+ /// * the `data_type` is not a Struct or a List,
+ /// * the `data_type` of extra key does not match with `data_type` of key
Review Comment:
I updated this in [c97313f](https://github.com/apache/arrow-datafusion/pull/7220/commits/c97313f476c0e068ad283b548805b6c8185e0874) -- what do you think?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220#discussion_r1287054403
##########
datafusion/expr/src/field_util.rs:
##########
@@ -20,64 +20,65 @@
use arrow::datatypes::{DataType, Field};
use datafusion_common::{plan_err, DataFusionError, Result, ScalarValue};
-pub enum GetFieldAccessCharacteristic {
- /// returns the field `struct[field]`. For example `struct["name"]`
+/// Types of the field access expression of a nested type, such as `Field` or `List`
+pub enum GetFieldAccessSchema {
+ /// Named field, For example `struct["name"]`
NamedStructField { name: ScalarValue },
- /// single list index
- // list[i]
+ /// Single list index, for example: `list[i]`
ListIndex { key_dt: DataType },
- /// list range `list[i:j]`
+ /// List range, for example `list[i:j]`
ListRange {
start_dt: DataType,
stop_dt: DataType,
},
}
-/// Returns the field access indexed by `key` and/or `extra_key` from a [`DataType::List`] or [`DataType::Struct`]
-/// # Error
-/// Errors if
-/// * the `data_type` is not a Struct or a List,
-/// * the `data_type` of extra key does not match with `data_type` of key
-/// * there is no field key is not of the required index type
-pub fn get_indexed_field(
- data_type: &DataType,
- field_characteristic: &GetFieldAccessCharacteristic,
-) -> Result<Field> {
- match field_characteristic {
- GetFieldAccessCharacteristic::NamedStructField{ name } => {
- match (data_type, name) {
- (DataType::Struct(fields), ScalarValue::Utf8(Some(s))) => {
- if s.is_empty() {
- plan_err!(
- "Struct based indexed access requires a non empty string"
- )
- } else {
- let field = fields.iter().find(|f| f.name() == s);
- field.ok_or(DataFusionError::Plan(format!("Field {s} not found in struct"))).map(|f| f.as_ref().clone())
+impl GetFieldAccessSchema {
+ /// Returns the schema [`Field`] from a [`DataType::List`] or
+ /// [`DataType::Struct`] indexed by this structure
+ ///
+ /// # Error
+ /// Errors if
+ /// * the `data_type` is not a Struct or a List,
+ /// * the `data_type` of extra key does not match with `data_type` of key
+ /// * there is no field key is not of the required index type
Review Comment:
```suggestion
/// # Error
/// Errors if
/// * the `data_type` is not a Struct or a List,
/// * the `data_type` of the name/index/start-stop do not match a supported index type
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7220: Minor: rename `GetFieldAccessCharacteristic` and add docs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7220:
URL: https://github.com/apache/arrow-datafusion/pull/7220#issuecomment-1668465830
CI is failing due to https://github.com/apache/arrow-datafusion/issues/7221
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org