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

[Domain-list] hostname in response is not unique #1120 #1121

Merged
merged 2 commits into from
May 4, 2024

Conversation

lydiapuric
Copy link
Collaborator

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

[Domain-list] hostname in response is not unique #1120

Thanks for reviewing!

@lydiapuric lydiapuric requested a review from langswei May 3, 2024 09:39
Copy link
Collaborator

@langswei langswei left a comment

Choose a reason for hiding this comment

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

Good catch. I wonder if maybe line 49 is where this was introduced, and an earlier (read: more appropriate) place to add DISTINCT. I have noticed the @v3 query sometimes times out now. Anyways, approving and you can choose to merge as-is or investigate further. You are the only consumer of this query, as far as I know.

@lydiapuric
Copy link
Collaborator Author

@langswei Just checked, thx. You are right, the DISTINCT in earlier place in line 49 is the right place to be, adjusting.

Copy link

github-actions bot commented May 4, 2024

This PR will trigger no release when merged.

@lydiapuric lydiapuric merged commit 594329a into main May 4, 2024
10 checks passed
@lydiapuric lydiapuric deleted the domain-list-unique-hostname branch May 4, 2024 03:49
@langswei
Copy link
Collaborator

langswei commented May 4, 2024

@lydiapuric I still see duplicates, but at least it's faster. Perhaps the answer is to have DISTINCT in both places. I will also speculate that this problem will go away over time -- earlier records did not have p###-e###, newer records do. Eventually the old ones will fall out of the query, but not until 2025 sometime so it's probably worth fixing.

@lydiapuric
Copy link
Collaborator Author

@langswei, I don't see duplicates. I made a count by hostname at the end. It looks good to me. and regarding records which did not have p###-e###, newer records do: in line 54 restrict to pattern and in line 97 i do only a join at hostname. so even if prior records have not the pattern, they will get assigned to the new cs values.

@trieloff
Copy link
Contributor

trieloff commented May 7, 2024

🎉 This PR is included in version 3.28.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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