You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "xanderbailey (via GitHub)" <gi...@apache.org> on 2026/08/28 10:00:25 UTC
[PR] refactor(scan): extract shared scan config [iceberg-rust]
xanderbailey opened a new pull request, #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093
## Which issue does this PR close?
- Working towards #2152.
## What changes are included in this PR?
Extract the existing table scan construction logic into an internal `ScanConfig` and `build_table_scan` function so it can be reused by other scan builders.
This is a behavior-preserving refactor. `TableScanBuilder` still resolves the same snapshot and schema, and manifest planning and the public API are unchanged.
This change was extracted from #2997 to let the shared scan construction be reviewed independently from incremental append scan semantics.
## Are these changes tested?
- `cargo fmt --all -- --check`
- `cargo test -p iceberg scan::tests --lib` (56 passed)
- `cargo clippy -p iceberg --lib --all-features -- -D warnings`
## AI Disclosure
Codex assisted with extracting the existing scan construction logic, validation, and drafting this PR. I reviewed the complete diff and verified the tests and lint checks listed above.
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3896713173
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +60,79 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn collect_scan_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
Review Comment:
Didn't want to make any functional changed during a refactor but let me give it a go.
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3880117736
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +64,103 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn projected_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .map(|f| f.name.clone())
+ .collect()
+ });
+
+ for column_name in column_names.iter() {
+ if is_metadata_column_name(column_name) {
+ field_ids.push(get_metadata_field_id(column_name)?);
+ continue;
+ }
+
+ let field_id = resolve_field_id(schema, column_name, case_sensitive).ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Column {column_name} not found in table. Schema: {schema}"),
+ )
+ })?;
+
+ schema
+ .as_struct()
+ .field_by_id(field_id)
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "Column {column_name} is not a direct child of schema but a nested field, which is not supported now. Schema: {schema}"
+ ),
+ )
+ })?;
+
+ field_ids.push(field_id);
+ }
+
+ Ok(field_ids)
+}
+
+fn bind_scan_predicate(
+ schema: &SchemaRef,
+ predicate: Option<&Predicate>,
+ case_sensitive: bool,
+) -> Result<Option<Arc<BoundPredicate>>> {
+ predicate
+ .map(|predicate| predicate.bind(schema.clone(), case_sensitive))
+ .transpose()
+ .map(|predicate| predicate.map(Arc::new))
+}
+
+fn table_name_mapping(table: &Table) -> Result<Option<Arc<NameMapping>>> {
Review Comment:
This is no longer necessary, you should use the new TableProperties api
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +64,103 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn projected_field_ids(
Review Comment:
```suggestion
fn collect_scan_field_ids(
```
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +64,103 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn projected_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .map(|f| f.name.clone())
+ .collect()
+ });
+
+ for column_name in column_names.iter() {
+ if is_metadata_column_name(column_name) {
+ field_ids.push(get_metadata_field_id(column_name)?);
+ continue;
+ }
+
+ let field_id = resolve_field_id(schema, column_name, case_sensitive).ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Column {column_name} not found in table. Schema: {schema}"),
+ )
+ })?;
+
+ schema
+ .as_struct()
+ .field_by_id(field_id)
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "Column {column_name} is not a direct child of schema but a nested field, which is not supported now. Schema: {schema}"
+ ),
+ )
+ })?;
+
+ field_ids.push(field_id);
+ }
+
+ Ok(field_ids)
+}
+
+fn bind_scan_predicate(
+ schema: &SchemaRef,
+ predicate: Option<&Predicate>,
+ case_sensitive: bool,
+) -> Result<Option<Arc<BoundPredicate>>> {
+ predicate
+ .map(|predicate| predicate.bind(schema.clone(), case_sensitive))
+ .transpose()
+ .map(|predicate| predicate.map(Arc::new))
+}
+
+fn table_name_mapping(table: &Table) -> Result<Option<Arc<NameMapping>>> {
+ Ok(table
+ .metadata()
+ .properties()
+ .get(DEFAULT_SCHEMA_NAME_MAPPING)
+ .map(|raw| {
+ serde_json::from_str::<NameMapping>(raw).map_err(|error| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping"
+ ),
+ )
+ .with_source(error)
+ })
+ })
+ .transpose()?
+ .map(Arc::new))
+}
+
+fn projected_partition_type(
+ table: &Table,
+ schema: &Schema,
+ field_ids: &[i32],
+) -> Result<Option<Arc<StructType>>> {
+ if !field_ids.contains(&RESERVED_FIELD_ID_PARTITION) {
+ return Ok(None);
+ }
+
+ compute_unified_partition_type(
+ table
+ .metadata()
+ .partition_specs_iter()
+ .map(|spec| spec.as_ref()),
+ schema,
+ )
+ .map(Arc::new)
+ .map(Some)
Review Comment:
I perfer to make this part of `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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3896797053
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +60,79 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn collect_scan_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
Review Comment:
[eb33947](https://github.com/apache/iceberg-rust/pull/3093/commits/eb33947670914abf36c4efb1494c7c0296843caa)
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#issuecomment-5451709724
Perhaps this is closer to what you're looking for @blackmwk
--
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] refactor(scan): extract shared scan config [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#issuecomment-5451432279
Hopefully not a controversial @blackmwk but means I can reuse this for append scans
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3892638524
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -32,6 +34,18 @@ fn parse_location_property(path: &str) -> Result<String> {
Ok(strip_trailing_slash(path).to_string())
}
+fn parse_name_mapping(value: &str) -> Result<Arc<NameMapping>> {
Review Comment:
How about implement `FromStr` for NameMapping?
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +60,79 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn collect_scan_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
Review Comment:
I don't think we need to collect to vec, also this is quite inefficient, if column_names is none, just return field id directly.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -336,6 +350,15 @@ pub struct TableProperties {
getter
)]
write_object_storage_partitioned_paths: bool,
+ /// The table's default name mapping, used to assign field ids when reading data files
+ /// that carry no field id metadata. `None` if `schema.name-mapping.default` is not set.
+ #[property(
+ key = Self::PROPERTY_DEFAULT_NAME_MAPPING,
+ default = None,
+ parse_with = parse_name_mapping,
+ getter
+ )]
+ default_name_mapping: Option<Arc<NameMapping>>,
Review Comment:
```suggestion
default_name_mapping: Option<NameMapping>,
```
The `TableProperties` is now a view of hashmap, I think it should be caller's responsibility to store it in Arc.
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3896710206
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -336,6 +350,15 @@ pub struct TableProperties {
getter
)]
write_object_storage_partitioned_paths: bool,
+ /// The table's default name mapping, used to assign field ids when reading data files
+ /// that carry no field id metadata. `None` if `schema.name-mapping.default` is not set.
+ #[property(
+ key = Self::PROPERTY_DEFAULT_NAME_MAPPING,
+ default = None,
+ parse_with = parse_name_mapping,
+ getter
+ )]
+ default_name_mapping: Option<Arc<NameMapping>>,
Review Comment:
[5853c0e](https://github.com/apache/iceberg-rust/pull/3093/commits/5853c0e6ad0d121c9271e3f4c1d14d1ba3479e4d)
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -32,6 +34,18 @@ fn parse_location_property(path: &str) -> Result<String> {
Ok(strip_trailing_slash(path).to_string())
}
+fn parse_name_mapping(value: &str) -> Result<Arc<NameMapping>> {
Review Comment:
[5853c0e](https://github.com/apache/iceberg-rust/pull/3093/commits/5853c0e6ad0d121c9271e3f4c1d14d1ba3479e4d)
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#discussion_r3880515874
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -61,6 +64,103 @@ fn resolve_field_id(schema: &Schema, column_name: &str, case_sensitive: bool) ->
}
}
+fn projected_field_ids(
+ schema: &Schema,
+ column_names: Option<&[String]>,
+ case_sensitive: bool,
+) -> Result<Vec<i32>> {
+ let mut field_ids = vec![];
+ let column_names = column_names.map(<[String]>::to_vec).unwrap_or_else(|| {
+ schema
+ .as_struct()
+ .fields()
+ .iter()
+ .map(|f| f.name.clone())
+ .collect()
+ });
+
+ for column_name in column_names.iter() {
+ if is_metadata_column_name(column_name) {
+ field_ids.push(get_metadata_field_id(column_name)?);
+ continue;
+ }
+
+ let field_id = resolve_field_id(schema, column_name, case_sensitive).ok_or_else(|| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!("Column {column_name} not found in table. Schema: {schema}"),
+ )
+ })?;
+
+ schema
+ .as_struct()
+ .field_by_id(field_id)
+ .ok_or_else(|| {
+ Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "Column {column_name} is not a direct child of schema but a nested field, which is not supported now. Schema: {schema}"
+ ),
+ )
+ })?;
+
+ field_ids.push(field_id);
+ }
+
+ Ok(field_ids)
+}
+
+fn bind_scan_predicate(
+ schema: &SchemaRef,
+ predicate: Option<&Predicate>,
+ case_sensitive: bool,
+) -> Result<Option<Arc<BoundPredicate>>> {
+ predicate
+ .map(|predicate| predicate.bind(schema.clone(), case_sensitive))
+ .transpose()
+ .map(|predicate| predicate.map(Arc::new))
+}
+
+fn table_name_mapping(table: &Table) -> Result<Option<Arc<NameMapping>>> {
+ Ok(table
+ .metadata()
+ .properties()
+ .get(DEFAULT_SCHEMA_NAME_MAPPING)
+ .map(|raw| {
+ serde_json::from_str::<NameMapping>(raw).map_err(|error| {
+ Error::new(
+ ErrorKind::DataInvalid,
+ format!(
+ "Failed to parse table property {DEFAULT_SCHEMA_NAME_MAPPING} as a NameMapping"
+ ),
+ )
+ .with_source(error)
+ })
+ })
+ .transpose()?
+ .map(Arc::new))
+}
+
+fn projected_partition_type(
+ table: &Table,
+ schema: &Schema,
+ field_ids: &[i32],
+) -> Result<Option<Arc<StructType>>> {
+ if !field_ids.contains(&RESERVED_FIELD_ID_PARTITION) {
+ return Ok(None);
+ }
+
+ compute_unified_partition_type(
+ table
+ .metadata()
+ .partition_specs_iter()
+ .map(|spec| spec.as_ref()),
+ schema,
+ )
+ .map(Arc::new)
+ .map(Some)
Review Comment:
Since it's a metadata only operation, I moved it to TableMetadata rather than Table, hope that works?
--
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] refactor(scan): extract shared scan config [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093#issuecomment-5451595204
I don't think this is the right direction to go. For incremental scan, what's required should be extracting out utilies like unifying partition specs, rather than reusing such a config.
--
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] refactor(scan): extract reusable scan functions [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #3093:
URL: https://github.com/apache/iceberg-rust/pull/3093
--
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