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

Remove duplicate sample data as id 90943e30-9a47-11e8-b64d-95841ca0b2… #5591

Closed
wants to merge 2 commits into from

Conversation

zhyuanqi
Copy link
Collaborator

@zhyuanqi zhyuanqi commented Dec 9, 2023

Description

Remove duplicate sample data as id 90943e30-9a47-11e8-b64d-95841ca0b247 appears twice on line 273 and 404. Duplicate entry.

Issues Resolved

No related issue with it.

Testing the changes

Since we only delete old record of id 90943e30-9a47-11e8-b64d-95841ca0b247. No record still exists on line 404. Visualization stays the same

Check List

  • All tests pass
    • [N] yarn test:jest (Two tests failed in my local. log_rotator.test.ts and create_os_package_tasks.test.ts. I don't think they are related to my change though)
    • [Y] yarn test:jest_integration
  • [N] New functionality includes testing.
  • [N] New functionality has been documented.
  • [N] Update CHANGELOG.md
  • [N] Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e7a2ba) 66.93% compared to head (2514d28) 66.98%.
Report is 20 commits behind head on main.

❗ Current head 2514d28 differs from pull request most recent head e7bf315. Consider uploading reports for the commit e7bf315 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5591      +/-   ##
==========================================
+ Coverage   66.93%   66.98%   +0.04%     
==========================================
  Files        3293     3293              
  Lines       63285    63287       +2     
  Branches    10061    10062       +1     
==========================================
+ Hits        42362    42394      +32     
+ Misses      18459    18453       -6     
+ Partials     2464     2440      -24     
Flag Coverage Δ
Linux_1 35.24% <0.00%> (-0.01%) ⬇️
Linux_2 55.21% <0.00%> (?)
Linux_3 43.78% <100.00%> (?)
Linux_4 35.34% <0.00%> (?)
Windows_1 35.27% <0.00%> (-0.01%) ⬇️
Windows_2 55.17% <0.00%> (-0.01%) ⬇️
Windows_3 43.80% <100.00%> (+<0.01%) ⬆️
Windows_4 35.34% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

manasvinibs
manasvinibs previously approved these changes Dec 13, 2023
@SuZhou-Joe
Copy link
Member

  1. Firstly, could you please fix the DCO error?
  2. Change log entry is missing.
  3. Could you please confirm that if the duplicate entry was added by mistake or by intention(as it may used for checking if OpenSearch can filter the duplicate import in bulk)? If you already know the answer, please add that in your PR's description so that we can safely approve this PR.

@SuZhou-Joe SuZhou-Joe self-assigned this Dec 14, 2023
@joshuarrrr
Copy link
Member

Could you please confirm that if the duplicate entry was added by mistake or by intention(as it may used for checking if OpenSearch can filter the duplicate import in bulk)? If you already know the answer, please add that in your PR's description so that we can safely approve this PR.

It looks like this was my mistake, introduced in #4339, so I can vouch for it being unintentional 😁

I am interested to understand what error or issues it causes - @zhyuanqi how did you happen to discover it?

@zhyuanqi
Copy link
Collaborator Author

zhyuanqi commented Jan 8, 2024

Hi I will add the change log and signoff in the next revision. Thanks.
So we found out this error when we try to import the sample data to S3 use id as key. It showed duplicate key. This is how to found out about this error.

…opensearch-project#5577)

* [BUG][Data] Support for custom filters with heterogeneous data fields

When enabling the advanced setting `courier:ignoreFilterIfFieldNotInIndex`
Custom OpenSearch Query DSL filters could technically be applied to index
patterns that map to indices that are not exactly the same. Since the
custom query filter is a user input then users can really type anything
that they need. Or any field that they know is present but we do not know
for sure.

Therefore, we can check if the id which is the index pattern title to check
if we should apply the filter or not.

Issue resolved:
https://github.com/opensearch-project/dashboards-visualizations/issues/281

I believe issue:
opensearch-project#5423

Should closed as that is expected functionality.

Signed-off-by: Kawika Avilla <[email protected]>

* [Cleanup] utilize the same helper function

Originally when implementing the fix the historical comment caused concern about
potential breaking changes.

But after discussion, we decided it is more clear to consolidate the helper functions.

Signed-off-by: Kawika Avilla <[email protected]>
…opensearch-project#5577)

* [BUG][Data] Support for custom filters with heterogeneous data fields

When enabling the advanced setting `courier:ignoreFilterIfFieldNotInIndex`
Custom OpenSearch Query DSL filters could technically be applied to index
patterns that map to indices that are not exactly the same. Since the
custom query filter is a user input then users can really type anything
that they need. Or any field that they know is present but we do not know
for sure.

Therefore, we can check if the id which is the index pattern title to check
if we should apply the filter or not.

Issue resolved:
https://github.com/opensearch-project/dashboards-visualizations/issues/281

I believe issue:
opensearch-project#5423

Should closed as that is expected functionality.

Signed-off-by: Kawika Avilla <[email protected]>

* [Cleanup] utilize the same helper function

Originally when implementing the fix the historical comment caused concern about
potential breaking changes.

But after discussion, we decided it is more clear to consolidate the helper functions.

Signed-off-by: Kawika Avilla <[email protected]>
@zhyuanqi
Copy link
Collaborator Author

zhyuanqi commented Jan 8, 2024

Close this pull request as created a new one #5668

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.

5 participants