fix(engine): update workflow paths and output handling in feature transformer #593
Conversation
|
Warning Rate limit exceeded@miseyu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request includes updates to 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: 1
🧹 Outside diff range and nitpick comments (6)
engine/runtime/examples/plateau/testdata/workflow/data-convert/01-bldg/workflow.yml (1)
64-68: Consider adding documentation for the AttributeFlattener node.To improve maintainability, consider adding a comment explaining:
- The purpose of attribute flattening in this workflow
- The expected input and output data structure
- Any specific transformation rules being applied
+ # Flattens complex CityGML attributes into a simplified structure + # Input: CityGML feature attributes + # Output: Flattened attribute structure - id: 1bd42875-4b8c-4439-82dc-9554d9946b07 name: AttributeFlattener type: action action: PLATEAU.AttributeFlattenerengine/runtime/action-processor/src/feature/reader/citygml.rs (3)
178-183: LGTM! Consider adding debug logging for skipped entities.The added type checks improve the robustness of the code by properly filtering out non-feature objects. This is a good defensive programming practice.
Consider adding debug logging when entities are skipped to help with troubleshooting:
let nusamai_citygml::Value::Object(obj) = &ent.root else { + log::debug!("Skipping non-object entity: {:?}", ent.root); continue; }; let nusamai_citygml::object::ObjectStereotype::Feature { .. } = &obj.stereotype else { + log::debug!("Skipping non-feature object: {:?}", obj.stereotype); continue; };
Line range hint
156-159: Consider documenting the coordinate transformation.The in-place coordinate swapping (lat,lng -> lng,lat) is a critical operation that could benefit from a more detailed comment explaining why this transformation is necessary and what coordinate system we're converting from/to.
geom_store.vertices.iter_mut().for_each(|v| { - // Swap x and y (lat, lng -> lng, lat) + // Transform coordinates from CityGML's (lat, lng, height) to GeoJSON's (lng, lat, height) format + // This is necessary because CityGML uses a different coordinate order than most geospatial formats (v[0], v[1], v[2]) = (v[1], v[0], v[2]); });
Line range hint
184-187: Consider using structured error types.The current error handling uses string formatting which makes it harder to handle specific error cases programmatically. Consider creating specific error variants for different failure modes.
Example enhancement:
#[derive(Debug)] pub enum CityGmlError { GeometryConversionError(String), InvalidFeatureError(String), // ... other specific error types } // Then use it like: let geometry: Geometry = ent.try_into().map_err(|e| super::errors::FeatureProcessorError::FileCityGmlReader( CityGmlError::GeometryConversionError(e.to_string()) ) )?;engine/runtime/examples/plateau/testdata/workflow/quality-check/05-luse-urf/workflow.yml (1)
188-201: LGTM: Attribute mapping expressions are standardizedThe attribute mapping expressions now consistently use
env.get("__value")for accessing values, making the code more explicit and maintainable.Consider extracting common attribute mapping patterns into reusable templates or helper functions to reduce code duplication. For example:
# Define common mappers in a separate config common_mappers: - attribute: Index expr: env.get("__value").fileIndex - attribute: Folder expr: env.get("__value").udxDirs # ... other common mappers # Reference in nodes nodes: - name: AttributeMapperInvalidSurface type: action action: AttributeMapper with: mappers: !include common_mappersThis would make the workflow more maintainable and reduce the risk of inconsistencies between similar mapper configurations.
engine/runtime/action-plateau-processor/src/attribute_flattener.rs (1)
Line range hint
234-238: Potential Panic Due to Uncheckedunwrap()At line 236,
area_type_code_count.unwrap()is called without checking if it isSome. Ifarea_type_code_countisNone, this will cause a panic at runtime. It's important to handle theNonecase to avoid unexpected panics.Apply this diff to safely handle the
Nonecase:-if desc.is_none() || area_type_code.is_none() || area_type_code_count.unwrap() > 2 { +if desc.is_none() || area_type_code.is_none() || area_type_code_count.map_or(false, |count| count > 2) {This change ensures that if
area_type_code_countisNone, the condition gracefully handles it without panicking.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (8)
- engine/Cargo.toml (1 hunks)
- engine/runtime/action-plateau-processor/src/attribute_flattener.rs (8 hunks)
- engine/runtime/action-processor/src/feature/reader/citygml.rs (1 hunks)
- engine/runtime/examples/plateau/example_feature_transformer.rs (1 hunks)
- engine/runtime/examples/plateau/testdata/workflow/attribute_flatterner.yml (0 hunks)
- engine/runtime/examples/plateau/testdata/workflow/data-convert/01-bldg/workflow.yml (2 hunks)
- engine/runtime/examples/plateau/testdata/workflow/quality-check/05-luse-urf/workflow.yml (3 hunks)
- engine/runtime/runner/src/orchestrator.rs (1 hunks)
💤 Files with no reviewable changes (1)
- engine/runtime/examples/plateau/testdata/workflow/attribute_flatterner.yml
✅ Files skipped from review due to trivial changes (2)
- engine/runtime/examples/plateau/example_feature_transformer.rs
- engine/runtime/runner/src/orchestrator.rs
🔇 Additional comments (12)
engine/runtime/examples/plateau/testdata/workflow/data-convert/01-bldg/workflow.yml (2)
122-126: LGTM! Edge configurations are properly defined.The new edges correctly integrate the AttributeFlattener node into the workflow, maintaining proper data flow between FeatureReader and VerticalReprojector nodes.
64-68: Verify if the AttributeFlattener node requires configuration parameters.The node definition appears to be missing a
withsection for configuration. Other action nodes in this workflow include configuration parameters, and the related Rust changes mentionFLATTEN_PREFIXES. Consider whether any parameters should be exposed here.✅ Verification successful
Let me search for the AttributeFlattener implementation to understand if it requires any configuration parameters.
Based on the Rust implementation, I can now provide a definitive response:
The AttributeFlattener node configuration is correctly defined
The node definition in the workflow.yml is complete and correct as is. Looking at the Rust implementation:
- The
parameter_schemamethod explicitly returnsNone, indicating this processor doesn't accept any configuration parameters- The processor is designed to work with default settings, automatically flattening CityGML attributes for building features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other AttributeFlattener configurations in the codebase rg -t yaml "action: PLATEAU.AttributeFlattener" -A 10Length of output: 1303
Script:
#!/bin/bash # Search for AttributeFlattener implementation in Rust files rg -t rust "AttributeFlattener" -A 10Length of output: 9002
engine/Cargo.toml (1)
64-68: Verify the impact of breaking changes in nusamai dependencies.The update from v0.3.1 to v0.4.0 represents a breaking change according to semver. The changes appear coordinated with updates in attribute processing and workflow configurations.
Let's verify the changes:
✅ Verification successful
Let me gather more information about potential breaking changes by checking for any modifications in the usage of these dependencies.
Based on the gathered information, I can now provide a final response:
Breaking changes in nusamai v0.4.0 have been properly handled
The update from v0.3.1 to v0.4.0 has been consistently applied across all nusamai dependencies, and the codebase shows proper integration:
- All nusamai dependencies are consistently using v0.4.0
- The v0.3.1 references found are unrelated (in go.sum for different packages)
- Usage patterns show proper imports and integration across multiple components:
- Geometry handling (types, projections, transformations)
- CityGML processing
- MVT (Map Vector Tiles) functionality
- Coordinate system operations
Recent commits also show a history of coordinated nusamai dependency updates, indicating this is part of a planned upgrade process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all nusamai dependencies are consistently updated and check for any remaining v0.3.1 references # Test 1: Check for any remaining v0.3.1 references echo "Checking for any remaining v0.3.1 references..." rg "v0.3.1" # Test 2: Verify that all nusamai dependencies are at v0.4.0 echo "Verifying nusamai dependency versions..." rg 'nusamai-[a-z]+.*tag = "v[0-9]+\.[0-9]+\.[0-9]+"'Length of output: 1260
Script:
#!/bin/bash # Check for any changes in the usage of nusamai dependencies echo "Checking for changes in nusamai usage patterns..." rg -A 2 "use nusamai_" --type rust # Check git history for related changes echo -e "\nChecking recent commits for nusamai-related changes..." git log --grep="nusamai" --since="1 month ago" --pretty=format:"%h %s" # Look for potential breaking changes in error handling or type signatures echo -e "\nChecking for potential breaking changes in function calls..." ast-grep --pattern 'nusamai_$_::$_($$$)'Length of output: 12433
engine/runtime/examples/plateau/testdata/workflow/quality-check/05-luse-urf/workflow.yml (1)
179-179: LGTM: Output path construction is simplified and consistentThe output path construction for error reporting files has been standardized using
file::join_path, making it more maintainable and consistent across the workflow.Also applies to: 216-216
engine/runtime/action-plateau-processor/src/attribute_flattener.rs (8)
Line range hint
468-488: Ensure Correct Handling of Risk Attributes for Building FeaturesThe implementation correctly extends
flattenedwith various risk attributes whenftype.as_str() == "bldg:Building". Ensure that all necessary risk attribute extraction methods are called and that they handle the attributes appropriately.
283-312: Recursive Flattening inflatten_attributeFunctionThe
flatten_attributefunction efficiently handles recursive flattening of nested attributes. The use of recursion and conditional checks ensures that attributes are flattened correctly based on their structure.
551-556: Extending Feature Attributes with New CityGML AttributesIn lines 551-556, the code extends
feature.attributeswithnew_city_gml_attribute. The mapping fromStringkeys toAttributeis correctly handled, and the attributes are properly cloned.
581-585: Proper Use of Unused Parameters infinishMethodThe
finishmethod correctly includes unused parameters_ctxand_fw, prefixed with underscores to indicate intentional non-use. This follows Rust conventions and prevents compiler warnings.
564-574: Efficient Removal of Unnecessary AttributesThe loop from lines 564-574 efficiently removes attributes that are not needed. The conditions check for specific prefixes and suffixes to identify attributes to remove.
19-90: Initialization ofFLATTEN_PREFIXESThe static
FLATTEN_PREFIXESis initialized usingLazyand correctly populates theHashSetwith all necessary attribute prefixes. The use ofvec!and iteration ensures that both keys and combined keys are included.
135-135:⚠️ Potential issueRedundant
.getCall on the Same KeyIn line 135, the
.get("uro:rank")is called twice in theor_elsechain. This redundancy does not serve any purpose and might be a copy-paste error. Consider removing the redundant call or replacing it with the intended key.Apply this diff to correct the redundancy:
-let rank = risk_map - .get("uro:rank") - .or_else(|| risk_map.get("uro:rank")); +let rank = risk_map.get("uro:rank");Or, if a different key was intended:
-let rank = risk_map - .get("uro:rank") - .or_else(|| risk_map.get("uro:rank")); +let rank = risk_map + .get("uro:rank") + .or_else(|| risk_map.get("uro:rankOrg"));Likely invalid or redundant comment.
338-340:⚠️ Potential issueHandle Possible
NoneValue When Unwrappingnum_lodIn the
update_max_lodmethod, unwrappingnum_lod.as_i64().unwrap()without checking ifas_i64()returnsSomecan lead to a panic if it returnsNone. To ensure robustness, handle theNonecase appropriately.Apply this diff to prevent potential panic:
-if num_lod.as_i64().unwrap() > 0 { +if let Some(num_lod_i64) = num_lod.as_i64() { + if num_lod_i64 > 0 { if self.max_lod < lod { self.max_lod = lod; } if gml_max_lod < lod { gml_max_lod = lod; self.gml_path_to_max_lod .insert(gml_path.clone(), gml_max_lod); } + } +}This modification checks if
num_lod.as_i64()returnsSomebefore unwrapping, ensuring safe execution.Likely invalid or redundant comment.
engine/runtime/action-plateau-processor/src/attribute_flattener.rs
Outdated
Show resolved
Hide resolved
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
New Features
nusamaipackages, enhancing functionality.AttributeFlatteneraction in the workflow for improved attribute processing.Bug Fixes
Documentation
Chores
FEATURE_FLUSH_THRESHOLDto optimize feature processing within the runtime environment.