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

Add AnalyzerOptions to Analyzer serialize / deserialize logic #597

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kchaturvedi
Copy link

@kchaturvedi kchaturvedi commented Jan 31, 2025

Issue #, if available: #596

Description of changes:
This PR addresses a discrepancy when using a FileSystemMetricsRepository with an Analyzer that has analyzerOptions set.

Given an analyzer which has analyzerOptions configured:

Compliance("rule1", "att1 > 3", columns = List("att1"),
  analyzerOptions = Some(AnalyzerOptions(
    nullBehavior = NullBehavior.Ignore,
    filteredRow = FilteredRowOutcome.NULL
  ))
)

Because the FileSystemMetricsRepository.get() function compares the entire analyzer object, it fails to match the current analyzer (with analyzerOptions) to any historical metric analyzer.

This issue occurs due to the following:

  • When the results are persisted to a FileSystemMetricsRepository, the AnalysisResultSerde.serialize function does not add analyzerOptions to the result object.

  • Similarly, when AnalysisResultSerde.deserialize pulls historic metrics from a file, it does not parse analyzerOptions even if they were present.

This issue means that no matter how many historical metrics are in the metrics repository file, none would be matched to the current analyzer, which would result in the check failing due to insufficient historical metrics. The current workaround would be to avoid using analyzerOptions if using FileSystemMetricsRepository, or switch to using InMemoryMetricsRepository

The fix I propose in this PR is to add the optional AnalyzerOptions object to the serializer and deserializer functions so that they may be persisted and fetched properly. This will enable the FileSystemMetricsRepository.get() function to successfully compare two analyzer objects and find an exact match.

Note that there is no issue using an InMemoryMetricsRepository as there is no serialization/deserialization happening in that case.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kchaturvedi kchaturvedi changed the title Add analyzer options to repository Serialize/deserialize AnalyzerOptions in AnalysisResultSerde Jan 31, 2025
@kchaturvedi kchaturvedi changed the title Serialize/deserialize AnalyzerOptions in AnalysisResultSerde Add AnalyzerOptions to Analyzer serialize / deserialize logic Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant