Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVRO-3917: [Rust] take field aliases into account when calculating schema compatibilities #2633

Conversation

marcosschroh
Copy link
Contributor

@marcosschroh marcosschroh commented Dec 11, 2023

What is the purpose of the change

  • Take field aliases are not taken into account when calculating schema compatibility AVRO-3917

Verifying this change

This change is partially covered by existing tests, such as avro_3894_take_aliases_into_account_when_serializing_for_schema_compatibility, however it does not cover all the aliases use cases in a proper way. With the changes that I am adding then it will fix the issue and we will covered the missing uses cases. The new test added is take_aliases_into_account_for_schema_compatibility

Documentation

  • Does this pull request introduce a new feature? (no)

@marcosschroh marcosschroh force-pushed the fix/check-aliases-when-calculating-schema-compatibility branch from b041bf6 to 8b87015 Compare December 11, 2023 13:27
@github-actions github-actions bot added the Rust label Dec 11, 2023
lang/rust/avro/src/schema_compatibility.rs Outdated Show resolved Hide resolved
lang/rust/avro/src/schema_compatibility.rs Outdated Show resolved Hide resolved
@marcosschroh marcosschroh force-pushed the fix/check-aliases-when-calculating-schema-compatibility branch from 8b87015 to fc8a269 Compare December 11, 2023 14:48
@marcosschroh
Copy link
Contributor Author

marcosschroh commented Dec 11, 2023

@martin-g I am seeing that the CI is failing because some packages can not be installed as they required rustc 1.70.0 or newer, for example clap builder and anstyle. Shall we update the rust version to 1.70.0?

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
@martin-g martin-g changed the title AVRO-3917: [Rust] take field aliases into account when calculating sc… AVRO-3917: [Rust] take field aliases into account when calculating schema compatibilities Dec 12, 2023
@martin-g martin-g merged commit e248e6b into apache:main Dec 12, 2023
15 checks passed
martin-g pushed a commit that referenced this pull request Dec 12, 2023
…hema compatibilities (#2633)

* AVRO-3917: [Rust] take field aliases into account when calculating schema compatibilities

* AVRO-3917: Minor improvements

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

---------

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>
(cherry picked from commit e248e6b)
@martin-g
Copy link
Member

Thank you, @marcosschroh !

// Check whether any of the possible fields names are in the writer schema.
// If the field was found, then it must have the exact same name with the writer field,
// otherwise we would have a false positive with the writers aliases
let mut position = None;
Copy link
Contributor

@woile woile Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can avoid this let mut with a find:

let position = fields_names.iter().find_map(|field_name| {
    if let Some(pos) = w_lookup.get(field_name) {
        if &w_fields[*pos].name == field_name {
            return Some(pos);
        }
    }
    return None
})

@marcosschroh marcosschroh deleted the fix/check-aliases-when-calculating-schema-compatibility branch February 16, 2024 16:19
RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
…hema compatibilities (apache#2633)

* AVRO-3917: [Rust] take field aliases into account when calculating schema compatibilities

* AVRO-3917: Minor improvements

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>

---------

Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Co-authored-by: Martin Tzvetanov Grigorov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants