refactor(engine): update parameter names and enhance CityGML processing #587
Conversation
WalkthroughThe changes in this pull request primarily focus on restructuring the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
engine/schema/actions.json (1)
1007-1012: LGTM. Consider updating documentation.The addition of the
flattenparameter to theFeatureReaderaction for thecitygmlformat is a good enhancement. It provides more flexibility in CityGML data processing while maintaining backward compatibility.Consider updating the user documentation to explain the purpose and usage of this new
flattenparameter for CityGML processing.engine/runtime/action-processor/src/feature/reader.rs (1)
187-187: Addpubvisibility toCompiledCommonReaderParamif neededThe
CompiledCommonReaderParamstruct is currently private to the module. If this struct needs to be accessed from other modules, consider adding thepubvisibility modifier.Apply this diff if external access is required:
-struct CompiledCommonReaderParam { +pub struct CompiledCommonReaderParam { expr: rhai::AST, }engine/runtime/action-source/src/file/reader/citygml.rs (1)
Line range hint
122-138: Encapsulate coordinate transformation into a separate functionThe coordinate swapping logic at lines 135-137 could be moved into a dedicated function or method. This would enhance code readability and make future maintenance or changes to the transformation logic easier.
engine/runtime/action-processor/src/feature/reader/citygml.rs (5)
24-28: Add documentation comments toCityGmlReaderParam.Consider adding Rust documentation comments (
///) to theCityGmlReaderParamstruct and its fields to enhance code readability and maintainability.Apply this diff to add documentation:
#[derive(Serialize, Deserialize, Debug, Clone, JsonSchema)] #[serde(rename_all = "camelCase")] +/// Parameters for reading CityGML data. pub struct CityGmlReaderParam { + /// If true, entities will be flattened. pub(super) flatten: Option<bool>, }
31-32: Ensure consistent error messages inread_citygml.The error handling within
read_citygmluses custom messages. To improve debugging and user feedback, ensure these messages are consistent and provide clear context for the errors.
64-71: Simplify theflattenparameter handling.Unwrapping
citygml_params.flattenbefore the function call improves readability and avoids repetitive code.Apply this diff:
let code_resolver = nusamai_plateau::codelist::Resolver::new(); let expr_engine = Arc::clone(&ctx.expr_engine); let feature = &ctx.feature; let scope = feature.new_scope(expr_engine.clone()); let city_gml_path = scope.eval_ast::<String>(¶ms.expr).map_err(|e| { super::errors::FeatureProcessorError::FileCityGmlReader(format!( "Failed to evaluate expr: {}", e )) })?; let flatten = citygml_params.flatten.unwrap_or(false); parse_tree_reader( &mut st, - citygml_params.flatten.unwrap_or(false), + flatten, base_url, ctx, fw, ) .map_err(|e| super::errors::FeatureProcessorError::FileCityGmlReader(format!("{:?}", e)))?;
Line range hint
77-188: Refactor to reduce code duplication in theflattenconditional blocks.The
if flattenandelseblocks share similar logic with minor differences. Refactoring can improve code maintainability and reduce duplication.You might consider extracting the common code into a separate function:
if flatten { let entities = FlattenTreeTransform::transform(entity); process_entities(entities, &mut transformer, &attributes, ctx, fw)?; } else { transformer.transform(&mut entity); process_entities(vec![entity], &mut transformer, &attributes, ctx, fw)?; } fn process_entities( entities: Vec<Entity>, transformer: &mut GeometricMergedownTransform, attributes: &HashMap<Attribute, AttributeValue>, ctx: ExecutorContext, fw: &mut dyn ProcessorChannelForwarder, ) -> Result<(), super::errors::FeatureProcessorError> { for mut entity in entities { transformer.transform(&mut entity); let geometry: Geometry = entity.try_into().map_err(|e| { super::errors::FeatureProcessorError::FileCityGmlReader(format!("{:?}", e)) })?; let mut feature: Feature = geometry.into(); feature.extend(attributes.clone()); fw.send(ctx.new_with_feature_and_port(feature, DEFAULT_PORT.clone())); } Ok(()) }
160-168: Ensure consistent attribute naming conventions.The keys in the
attributesHashMap (e.g.,"cityGmlAttributes","gmlName","gmlId","gmlRootId") should follow a consistent naming convention for clarity and to prevent potential issues during attribute access.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (7)
- engine/runtime/action-processor/src/feature/reader.rs (4 hunks)
- engine/runtime/action-processor/src/feature/reader/citygml.rs (3 hunks)
- engine/runtime/action-processor/src/feature/reader/csv.rs (1 hunks)
- engine/runtime/action-source/src/file/reader/citygml.rs (6 hunks)
- engine/runtime/action-source/src/file/reader/runner.rs (2 hunks)
- engine/runtime/examples/plateau/example_feature_transformer.rs (1 hunks)
- engine/schema/actions.json (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- engine/runtime/examples/plateau/example_feature_transformer.rs
🧰 Additional context used
🔇 Additional comments (9)
engine/runtime/action-processor/src/feature/reader/csv.rs (3)
11-11: LGTM: New import aligns with restructuring.The addition of
use super::CompiledCommonReaderParam;is consistent with the refactoring mentioned in the summary, whereCommonReaderParamreplacesCommonPropertySchema.
15-15: LGTM: Struct renamed for clarity.The renaming of
CsvPropertySchematoCsvReaderParamimproves code clarity by better reflecting its purpose as a parameter for the CSV reader.
21-22: LGTM: Function signature updated to match new parameter types.The
read_csvfunction signature has been correctly updated to useCompiledCommonReaderParamandCsvReaderParam, aligning with the struct renaming and new parameter types.To ensure consistency across the codebase, please run the following script to check for any remaining references to the old types:
✅ Verification successful
Verified: No remaining references to
CompiledCommonPropertySchemaorCsvPropertySchemawere found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to old types # Test 1: Search for CompiledCommonPropertySchema echo "Searching for CompiledCommonPropertySchema:" rg "CompiledCommonPropertySchema" # Test 2: Search for CsvPropertySchema echo "Searching for CsvPropertySchema:" rg "CsvPropertySchema" # Test 3: Search for calls to read_csv function echo "Searching for read_csv function calls:" ast-grep --pattern 'read_csv($$$)'Length of output: 352
engine/schema/actions.json (1)
1348-1353: LGTM. Ensure consistency across the codebase.The addition of the
flattenparameter to theFileReaderaction for thecitygmlformat is consistent with the changes inFeatureReader. This provides a unified approach to CityGML processing across different reader types.To ensure consistency, please run the following script to check if the
flattenparameter is used consistently across the codebase:✅ Verification successful
Please run the following script to verify the consistent usage of the
flattenparameter across all action schema files:
'Flatten' parameter consistency verified across action schemas.
All relevant reader actions include the
flattenparameter for CityGML processing.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of the 'flatten' parameter across the codebase. # Test: Search for 'flatten' usage in relation to CityGML processing rg -i 'flatten.*citygml|citygml.*flatten' --type rustLength of output: 166
Script:
# #!/bin/bash # Description: Verify 'flatten' parameter consistency across all action schema JSON files. # Test: Search for 'flatten' parameter in all JSON action schema files rg -i '"flatten"\s*:' --type jsonLength of output: 143
engine/runtime/action-source/src/file/reader/runner.rs (2)
44-45: Maintain consistency in parameter handling across all file formatsThe
Citygmlvariant now includes apropertyfield with#[serde(flatten)], aligning it with theCsvandTsvvariants. This promotes consistency in how additional parameters are handled for different file formats, which is good practice.
112-117: Handle potential errors fromget_input_pathuniformlyIn the match arms for other file formats, if
get_input_pathreturns an error, it propagates up appropriately. Ensure that the error handling forCitygmlis consistent with the rest of the codebase, and that any errors fromget_input_pathare properly managed.Run the following script to check for consistent error handling:
✅ Verification successful
Consistent Error Handling Verified
All instances of
get_input_pathuniformly propagate errors using the?operator. No discrepancies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all uses of `get_input_path` handle errors consistently. # Search for all instances where `get_input_path` is called. rg 'let input_path = get_input_path\(&ctx, common_property\)\?;' # Check that the error handling after each call matches. # There should be no discrepancies in how errors are propagated after calling `get_input_path`.Length of output: 578
engine/runtime/action-processor/src/feature/reader.rs (1)
Line range hint
140-152: Ensure consistent usage ofCommonReaderParamThe struct
CommonPropertySchemahas been renamed toCommonReaderParam. Verify that all references toCommonPropertySchemaare updated throughout the codebase to prevent any inconsistencies or compilation errors.Run the following script to check for any remaining references:
engine/runtime/action-source/src/file/reader/citygml.rs (2)
133-138: Verify the coordinate swapping logic for accuracyThe code swaps the x and y coordinates of each vertex at lines 135-137. Ensure that this transformation accurately reflects the intended coordinate system conversion (e.g., from (lat, lng) to (lng, lat)). Misordering coordinates can lead to incorrect geometries and map rendering issues.
167-167: Optimize attribute cloning to improve performanceAt line 167,
attributes.clone()is called for each feature within the loop. If theattributesHashMap remains unchanged throughout the loop, clone it once before the loop and reuse the cloned version to avoid unnecessary overhead.[performance]
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
engine/schema/actions.json (2)
1007-1012: LGTM. Consider updating documentation.The addition of the optional
flattenparameter to theFeatureReaderprocessor for thecitygmlformat is a good enhancement. It provides more flexibility in processing CityGML data while maintaining backward compatibility.Consider updating the processor's description or adding inline comments to explain the purpose and effect of the
flattenparameter.
1007-1012: Summary: Consistent and backward-compatible enhancementThe addition of the
flattenparameter to bothFeatureReaderandFileReaderprocessors for CityGML format is a well-implemented enhancement. It provides more flexibility in data processing while maintaining backward compatibility. The consistent implementation across both readers ensures a uniform approach to handling CityGML data.Consider adding a brief explanation of the
flattenparameter's functionality in the schema descriptions for both processors to improve clarity for users of this schema.Also applies to: 1348-1353
engine/runtime/action-source/src/file/reader/citygml.rs (2)
23-27: Add documentation forCityGmlReaderParamand its fieldsProviding documentation comments for the
CityGmlReaderParamstruct and itsflattenfield will enhance code readability and maintainability.
150-158: Avoid shadowing theattributesvariableThe variable
attributesis redefined as aHashMap, which may cause confusion with the existingattributesderived fromentity.root.to_attribute_json(). Consider renaming one of the variables to improve clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/docs/mdbook/src/action.mdis excluded by!**/*.md
📒 Files selected for processing (7)
- engine/runtime/action-processor/src/feature/reader.rs (4 hunks)
- engine/runtime/action-processor/src/feature/reader/citygml.rs (3 hunks)
- engine/runtime/action-processor/src/feature/reader/csv.rs (1 hunks)
- engine/runtime/action-source/src/file/reader/citygml.rs (6 hunks)
- engine/runtime/action-source/src/file/reader/runner.rs (2 hunks)
- engine/runtime/examples/plateau/example_feature_transformer.rs (1 hunks)
- engine/schema/actions.json (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- engine/runtime/examples/plateau/example_feature_transformer.rs
🧰 Additional context used
🔇 Additional comments (13)
engine/runtime/action-processor/src/feature/reader/csv.rs (4)
11-11: LGTM: Import statement updated correctly.The import statement has been updated to reflect the renaming of
CompiledCommonPropertySchematoCompiledCommonReaderParam. This change is consistent with the refactoring mentioned in the PR objectives.
Line range hint
1-78: Summary: Successful refactoring of CSV reader component.The changes in this file successfully implement the refactoring mentioned in the PR objectives. The renaming of structs and updating of parameter types improve code clarity and consistency. The core CSV reading functionality remains intact, which is crucial for maintaining existing behavior.
Key points:
CsvPropertySchemarenamed toCsvReaderParamCompiledCommonPropertySchemareplaced withCompiledCommonReaderParam- Function parameters in
read_csvupdated accordinglyThese changes align well with the overall goal of refactoring the engine component and updating parameter names.
21-22: LGTM: Function parameters updated correctly.The parameter types in the
read_csvfunction have been properly updated to reflect the renamed structs:
params: &CompiledCommonReaderParamcsv_params: &CsvReaderParamThese changes maintain consistency with the struct renaming.
To ensure all calls to this function have been updated accordingly, please run the following verification:
#!/bin/bash # Description: Check for any remaining occurrences of old parameter types in function calls # Test: Search for read_csv function calls. Expect: All calls use new parameter types rg 'read_csv\s*\(' -A 3
15-15: LGTM: Struct renamed for clarity.The struct has been appropriately renamed from
CsvPropertySchematoCsvReaderParam, which better reflects its purpose as a parameter for the CSV reader.To ensure consistency across the codebase, please verify all occurrences of this struct have been updated:
✅ Verification successful
Verified: Struct renaming successful.
All instances of
CsvPropertySchemahave been correctly updated toCsvReaderParam, ensuring consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of CsvPropertySchema # Test: Search for CsvPropertySchema. Expect: No results rg 'CsvPropertySchema'Length of output: 2154
engine/schema/actions.json (1)
1348-1353: LGTM. Ensure consistent implementation.The addition of the optional
flattenparameter to theFileReaderprocessor for thecitygmlformat is consistent with the changes made to theFeatureReader. This ensures a uniform approach to processing CityGML data across different reader types.To ensure consistency, please verify that the implementation of the
flattenparameter in bothFeatureReaderandFileReaderbehaves identically for CityGML processing. Run the following script to check for any discrepancies:✅ Verification successful
Verified: Consistent implementation of
flattenparameter in both FeatureReader and FileReader.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent implementation of `flatten` parameter in FeatureReader and FileReader # Test: Search for the implementation of flatten parameter in both readers echo "Searching for flatten parameter implementation in FeatureReader:" rg -p 'impl.*FeatureReader.*flatten' engine/runtime/action-processor/src/feature/reader/ echo "\nSearching for flatten parameter implementation in FileReader:" rg -p 'impl.*FileReader.*flatten' engine/runtime/action-source/src/file/reader/Length of output: 448
Script:
#!/bin/bash # Description: Verify consistent implementation of `flatten` parameter in FeatureReader and FileReader with adjusted patterns # Test: Search for the usage and implementation of flatten parameter in both readers echo "Searching for 'flatten' parameter usage in FeatureReader:" rg 'flatten' engine/runtime/action-processor/src/feature/reader/ echo "\nSearching for 'flatten' parameter usage in FileReader:" rg 'flatten' engine/runtime/action-source/src/file/reader/Length of output: 1709
engine/runtime/action-processor/src/feature/reader.rs (2)
203-232: Confirm that updated parameters are correctly utilized in processingIn the
processmethod, ensure that the updatedcommon_paramandparamare correctly passed to the reading functions for each format. This is crucial to guarantee that the new parameter structures are utilized properly during processing.You can verify the correctness by reviewing the function signatures in
citygml.rsandcsv.rsto ensure they accept the updated parameters.
Line range hint
140-187: Ensure all references to renamed structs are updatedThe struct
CommonPropertySchemahas been renamed toCommonReaderParam, andCompiledCommonPropertySchematoCompiledCommonReaderParam. Verify that all references to the old struct names have been updated throughout the codebase to prevent compilation errors.Run the following script to check for any remaining references to the old struct names:
✅ Verification successful
All references to the old struct names have been successfully updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for occurrences of the old struct names in the codebase # Expect: No matches found for `CommonPropertySchema` or `CompiledCommonPropertySchema` rg 'CommonPropertySchema|CompiledCommonPropertySchema'Length of output: 56
Script:
#!/bin/bash # Description: Search for exact occurrences of the old struct names in the codebase # Expect: No matches found for `CommonPropertySchema` or `CompiledCommonPropertySchema` rg '\bCommonPropertySchema\b|\bCompiledCommonPropertySchema\b' --case-sensitive -A 2Length of output: 86
engine/runtime/action-source/src/file/reader/citygml.rs (1)
132-138: Verify the necessity of swapping vertex coordinatesSwapping the x and y coordinates of the vertices may lead to incorrect geometries if not intended. Please verify that this transformation is necessary for the data being processed and aligns with the coordinate system conventions.
engine/runtime/action-processor/src/feature/reader/citygml.rs (4)
24-28: Addition ofCityGmlReaderParamstruct is appropriateThe
CityGmlReaderParamstruct is well-defined with serialization and deserialization traits, enhancing the configurability of the CityGML reader.
31-32: Update ofread_citygmlfunction signature aligns with new parametersIncluding
citygml_paramsin theread_citygmlfunction signature accommodates the new parameters and maintains consistency in parameter passing.
64-71: Passingflattenparameter toparse_tree_readeris correctly implementedUsing
citygml_params.flatten.unwrap_or(false)safely handles the optionalflattenparameter, defaulting tofalsewhen not provided.
77-77: Update ofparse_tree_readerfunction signature is appropriateAdding the
flattenparameter to theparse_tree_readerfunction signature properly integrates the new functionality.engine/runtime/action-source/src/file/reader/runner.rs (1)
44-45: LGTM!Adding the
propertyfield to theCitygmlvariant maintains consistency with other variants likeCsvandTsv. The use of#[serde(flatten)]is appropriate for serializing and deserializing nested parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
engine/runtime/action-source/src/file/reader/runner.rs (1)
44-45: LGTM! Consider adding documentation for the newCitygmlvariant.The addition of the
Citygmlvariant to theFileReaderenum is well-structured and consistent with the existing code. It aligns with the PR objectives to enhance CityGML processing.Consider adding a brief documentation comment for the
Citygmlvariant to explain its purpose and usage, similar to other variants if they exist.engine/runtime/action-processor/src/feature/reader/citygml.rs (3)
30-32: LGTM: Updated function signature improves flexibility.The changes to the
read_citygmlfunction signature, including the newcitygml_paramsparameter and the updatedparamstype, enhance the configurability of the CityGML reading process. This aligns well with the PR objectives.Consider updating the error handling to include the new
citygml_paramsin error messages for better debugging:super::errors::FeatureProcessorError::FileCityGmlReader(format!( "Failed to evaluate expr: {}. CityGML params: {:?}", e, citygml_params ))
64-71: LGTM: Flattening option adds flexibility to CityGML processing.The addition of the
flattenparameter toparse_tree_readerand its usage inread_citygmlprovide a way to control the CityGML structure processing. This enhancement aligns with the PR objectives.Consider adding more context to the error message:
super::errors::FeatureProcessorError::FileCityGmlReader(format!( "Error parsing tree reader with flatten={}: {:?}", flatten, e ))Also applies to: 76-77
162-185: LGTM: Improved attribute handling and flexible entity processing.The use of
HashMapfor attributes and the addition of new metadata attributes (gmlName,gmlId,gmlRootId) enhance the feature representation. The conditional processing based on theflattenparameter provides flexibility in output structure, aligning with the PR objectives.Consider using
HashMap::with_capacityto potentially improve performance:let mut attributes = HashMap::with_capacity(4 + ctx.feature.attributes.len()); attributes.insert(Attribute::new("cityGmlAttributes"), attributes.into()); // ... insert other attributes attributes.extend(ctx.feature.attributes.clone());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- engine/runtime/action-processor/src/feature/reader/citygml.rs (4 hunks)
- engine/runtime/action-source/src/file/reader/citygml.rs (6 hunks)
- engine/runtime/action-source/src/file/reader/runner.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
engine/runtime/action-source/src/file/reader/runner.rs (1)
Line range hint
112-122: LGTM! TheCitygmlcase is well-implemented.The implementation of the
Citygmlcase in thestartmethod is consistent with other file format handlers and correctly uses the newpropertyfield. Great job implementing the suggestion from past reviews by passingstorage_resolverinstead ofctxtoread_citygml.engine/runtime/action-processor/src/feature/reader/citygml.rs (2)
2-2: LGTM: New imports and struct enhance functionality.The addition of
HashMap,FlattenTreeTransform, andJsonSchemaimports, along with the newCityGmlReaderParamstruct, provide the necessary components for the flattening feature and improved attribute handling. These changes align well with the PR objectives of enhancing CityGML processing.Also applies to: 8-10, 18-19, 24-28
171-171: 🛠️ Refactor suggestionConsider optimizing attribute cloning.
The unnecessary cloning of
ctx.feature.attributesnoted in a previous review is still present. This can have performance implications, especially for large attribute sets.Consider using a reference or moving the attributes instead of cloning:
let mut attributes = ctx.feature.attributes.clone(); attributes.insert(Attribute::new("cityGmlAttributes"), cityGml_attributes.into()); attributes.insert(Attribute::new("gmlName"), name); attributes.insert(Attribute::new("gmlId"), gml_id); attributes.insert(Attribute::new("gmlRootId"), AttributeValue::String(format!("root_{}", to_hash(base_url.as_str()))));This approach clones the attributes once and then modifies the cloned version, which may be more efficient.
engine/runtime/action-source/src/file/reader/citygml.rs (2)
2-25: LGTMThe introduction of the
CityGmlReaderParamstruct and the updates to include theflattenparameter enhance the flexibility of the code. Serialization and deserialization are correctly implemented, and the optionalflattenparameter is appropriately handled.
157-177: LGTMThe implementation of the
flattenparameter logic correctly handles the entity transformation based on its value. The use ofFlattenTreeTransform::transform(entity)whenflattenistrueand the subsequent processing are appropriately managed.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
flattenparameter forFeatureReaderandFileReaderprocessors, allowing for flexible data structure handling during processing.CityGmlReaderParamto include an optionalflattenfield for improved parsing control.Bug Fixes
read_citygmlfunction to utilize theflattenparameter, refining data processing.Documentation
flattenparameter in bothFeatureReaderandFileReader.