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-3939: [Rust] Make it possible to use custom schema equality comparators #2739

Merged
merged 14 commits into from
Feb 20, 2024

Conversation

martin-g
Copy link
Member

@martin-g martin-g commented Feb 15, 2024

AVRO-3939

What is the purpose of the change

  • Introduce a trait that could be implemented with a custom impl for comparing the equality of two schemas

Verifying this change

  • All new and old tests should pass

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? RustDoc

WIP

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Implement compare_fields()

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Add unit tests for the primitives

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g changed the title AVRO-3939: [Rust] Make it possible to use custom schema comparators AVRO-3939: [Rust] Make it possible to use custom schema equality comparators Feb 18, 2024
StructFieldEq

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g
Copy link
Member Author

@ZENOTME The PR is ready for review and test!
I will add more unit tests!

@martin-g martin-g marked this pull request as ready for review February 19, 2024 12:12
Fix clippy error with Rust nightly, e.g.:

```
error: the item `TryFrom` is imported redundantly
  --> avro/src/types.rs:34:5
   |
34 |     convert::TryFrom,
   |     ^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `TryFrom` is already defined here

error: could not compile `apache-avro` (lib) due to 9 previous errors
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
fix more Rust nightly clippy errors in tests

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g force-pushed the avro-3939-custom-schemata-comparators branch from ae14966 to 8d40852 Compare February 19, 2024 12:42
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
```
error: the item `RecordField` is imported redundantly
    --> avro/src/types.rs:1153:18
     |
1150 |     use super::*;
     |         -------- the item `RecordField` is already imported here
...
1153 |         schema::{RecordField, RecordFieldOrder},
     |                  ^^^^^^^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
…ogged messages

Not just against the last logged message

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g force-pushed the avro-3939-custom-schemata-comparators branch from fd8689d to a1ab3e1 Compare February 19, 2024 13:52
```
error: the item `FromIterator` is imported redundantly
  --> avro/benches/serde_json.rs:20:33
   |
20 | use std::{collections::HashMap, iter::FromIterator};
   |                                 ^^^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `FromIterator` is already defined here
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: could not compile `apache-avro` (bench "serde_json") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
It is much faster than SpecificationEq which serializes the schemas to
JSON and then compares them as strings

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@martin-g martin-g merged commit e038004 into main Feb 20, 2024
17 checks passed
@martin-g martin-g deleted the avro-3939-custom-schemata-comparators branch February 20, 2024 09:18
martin-g added a commit that referenced this pull request Feb 20, 2024
…arators (#2739)

* AVRO-3939: [Rust] Make it possible to use custom schema comparators

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Temporarily use StructFieldComparator as default

Implement compare_fields()

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Fix formatting and clippy issues

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Rename the trait and its impls from Comparator to Eq

Add unit tests for the primitives

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Add support for comparing the custom attributes to
StructFieldEq

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Add more unit tests

Fix clippy error with Rust nightly, e.g.:

```
error: the item `TryFrom` is imported redundantly
  --> avro/src/types.rs:34:5
   |
34 |     convert::TryFrom,
   |     ^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `TryFrom` is already defined here

error: could not compile `apache-avro` (lib) due to 9 previous errors
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRo-3939: Add more unit tests

fix more Rust nightly clippy errors in tests

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: More unit tests and better logging

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Fix rust nightly clippy error

```
error: the item `RecordField` is imported redundantly
    --> avro/src/types.rs:1153:18
     |
1150 |     use super::*;
     |         -------- the item `RecordField` is already imported here
...
1153 |         schema::{RecordField, RecordFieldOrder},
     |                  ^^^^^^^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Improve TestLogger's assert_logged to assert against all logged messages

Not just against the last logged message

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* Fix rust nightly clippy error

```
error: the item `FromIterator` is imported redundantly
  --> avro/benches/serde_json.rs:20:33
   |
20 | use std::{collections::HashMap, iter::FromIterator};
   |                                 ^^^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `FromIterator` is already defined here
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: could not compile `apache-avro` (bench "serde_json") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: More test assertions and better logging

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Use StructFieldEq impl by default

It is much faster than SpecificationEq which serializes the schemas to
JSON and then compares them as strings

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVro-3939: Format the code

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

---------

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
(cherry picked from commit e038004)
@ZENOTME
Copy link
Contributor

ZENOTME commented Feb 29, 2024

Sorry for being late. Thanks for this nice job! @martin-g

RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
…arators (apache#2739)

* AVRO-3939: [Rust] Make it possible to use custom schema comparators

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Temporarily use StructFieldComparator as default

Implement compare_fields()

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Fix formatting and clippy issues

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Rename the trait and its impls from Comparator to Eq

Add unit tests for the primitives

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Add support for comparing the custom attributes to
StructFieldEq

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Add more unit tests

Fix clippy error with Rust nightly, e.g.:

```
error: the item `TryFrom` is imported redundantly
  --> avro/src/types.rs:34:5
   |
34 |     convert::TryFrom,
   |     ^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `TryFrom` is already defined here

error: could not compile `apache-avro` (lib) due to 9 previous errors
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRo-3939: Add more unit tests

fix more Rust nightly clippy errors in tests

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: More unit tests and better logging

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Fix rust nightly clippy error

```
error: the item `RecordField` is imported redundantly
    --> avro/src/types.rs:1153:18
     |
1150 |     use super::*;
     |         -------- the item `RecordField` is already imported here
...
1153 |         schema::{RecordField, RecordFieldOrder},
     |                  ^^^^^^^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Improve TestLogger's assert_logged to assert against all logged messages

Not just against the last logged message

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* Fix rust nightly clippy error

```
error: the item `FromIterator` is imported redundantly
  --> avro/benches/serde_json.rs:20:33
   |
20 | use std::{collections::HashMap, iter::FromIterator};
   |                                 ^^^^^^^^^^^^^^^^^^
  --> /rustc/2bf78d12d33ae02d10010309a0d85dd04e7cff72/library/std/src/prelude/mod.rs:129:13
   |
   = note: the item `FromIterator` is already defined here
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: could not compile `apache-avro` (bench "serde_json") due to 1 previous error
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.
```

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: More test assertions and better logging

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVRO-3939: Use StructFieldEq impl by default

It is much faster than SpecificationEq which serializes the schemas to
JSON and then compares them as strings

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

* AVro-3939: Format the code

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>

---------

Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
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.

2 participants