Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
comphead merged PR #1643: URL: https://github.com/apache/datafusion-comet/pull/1643 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
comphead commented on PR #1643: URL: https://github.com/apache/datafusion-comet/pull/1643#issuecomment-2802258771 Thanks @andygrove for the review -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
andygrove commented on code in PR #1643:
URL: https://github.com/apache/datafusion-comet/pull/1643#discussion_r2042233446
##
native/core/src/execution/shuffle/row.rs:
##
@@ -1876,937 +1878,834 @@ fn make_builders(
(DataType::Boolean, DataType::Boolean) => {
let key_builder = downcast_builder!(BooleanBuilder,
key_builder);
let value_builder = downcast_builder!(BooleanBuilder,
value_builder);
-Box::new(MapBuilder::new(
-Some(map_fieldnames),
-*key_builder,
-*value_builder,
-))
+Box::new(
+MapBuilder::new(Some(map_field_names), *key_builder,
*value_builder)
+.with_values_field(Arc::clone(value_field)),
Review Comment:
note for other reviewers: this seems to be the main change
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
comphead commented on code in PR #1643: URL: https://github.com/apache/datafusion-comet/pull/1643#discussion_r2041190729 ## native/core/src/execution/shuffle/map.rs: ## @@ -2832,13 +2833,13 @@ pub fn append_map_elements( } #[allow(clippy::field_reassign_with_default)] -pub fn get_map_key_value_dt( +pub fn get_map_key_value_fields( Review Comment: refactored the method to return entire field instead of just datatype -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
codecov-commenter commented on PR #1643: URL: https://github.com/apache/datafusion-comet/pull/1643#issuecomment-2800074535 ## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1643?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 57.65%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`10e984c`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/10e984c901be470a6b3dfa6d6c81126bccc409b7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 140 commits behind head on main. Additional details and impacted files ```diff @@ Coverage Diff @@ ## main#1643 +/- ## + Coverage 56.12% 57.65% +1.53% - Complexity 976 1008 +32 Files 119 125 +6 Lines 1174312413 +670 Branches 2251 2331 +80 + Hits 6591 7157 +566 - Misses 4012 4072 +60 - Partials 1140 1184 +44 ``` [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1643?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat: Override MapBuilder values field with expected schema [datafusion-comet]
comphead commented on code in PR #1643:
URL: https://github.com/apache/datafusion-comet/pull/1643#discussion_r2041175431
##
native/core/src/execution/shuffle/row.rs:
##
@@ -904,7 +904,7 @@ pub(crate) fn append_field(
append_map_element!(StringBuilder, Decimal128Builder,
field);
}
(DataType::Utf8, DataType::Struct(_)) => {
-append_map_element!(StringBuilder, StructBuilder, field);
+//append_map_element!(StringBuilder, StructBuilder, field);
Review Comment:
Map arm branches takes too much of space in this file, proposing to move Map
arm branches into a separate file
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
