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

Feature/most recent unique #2570

Open
wants to merge 13 commits into
base: integration
Choose a base branch
from
Open

Conversation

ivakegg
Copy link
Collaborator

@ivakegg ivakegg commented Sep 18, 2024

No description provided.

@ivakegg ivakegg requested a review from jwomeara as a code owner September 18, 2024 20:20
@ivakegg ivakegg force-pushed the feature/mostRecentUnique branch from ca23bd7 to 33d30e5 Compare November 22, 2024 14:08
@ivakegg ivakegg force-pushed the feature/mostRecentUnique branch from da314e2 to 9dec45e Compare November 22, 2024 16:52
avgAGB
avgAGB previously approved these changes Jan 8, 2025
Copy link
Collaborator

Choose a reason for hiding this comment

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

no unit tests for this class

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm... perhaps not directly. I will see what I can do.

Comment on lines +82 to +84
put("TLD_FIELD_A", 4); // 3 record ids, 1 value
put("TLD_FIELD_B", 14); // 13 record ids, 1 value
put("TLD_FIELD_C", 5); // 4 record ids, 1 value
Copy link
Collaborator

Choose a reason for hiding this comment

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

What caused this assertion to change? Was the TLD aggregator not working before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really don't know why this changed.. I will figure it out however.

if (!this.uniqueFields.equals(uniqueFields)) {
this.uniqueFields = uniqueFields.clone();
log.info("Resetting unique fields on the unique transform");
this.bloom = BloomFilter.create(new ByteFunnel(), 500000, 1e-15);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since the bloom is only used when not using isMostRecent() can we avoid creating the bloom at all when that is set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably, I will see if I can do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants