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

Demonstrate that arrays of geo_shape can no longer be ingested #14196

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

Conversation

kurtmckee
Copy link

@kurtmckee kurtmckee commented Jun 11, 2024

Description

Note

This PR is not intended for merging as-is. It demonstrates issue #14193, which reports a regression in OpenSearch introduced in #4266 and #8301, and released in v2.9.0 and higher.

It is expected that OpenSearch maintainers can fetch this branch, add fix commits to the branch to resolve the underlying bug, and push directly to this PR. Thank you!

# Using the GitHub CLI
gh co 14196

# Using git
git remote add kurtmckee git@github.com:kurtmckee/pr-OpenSearch.git
git fetch kurtmckee
git checkout -b fix-arrays-of-geo_shape-issue-14193 kurtmckee/fix-arrays-of-geo_shape-issue-14193

Related Issues

Resolves #14193

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

This was first broken in commit ebe6f2c

Demonstrates opensearch-project#14193

Signed-off-by: Kurt McKee <contactme@kurtmckee.org>
Copy link
Contributor

❌ Gradle check result for ce13959: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Kurt McKee <contactme@kurtmckee.org>
@kurtmckee kurtmckee force-pushed the fix-arrays-of-geo_shape-issue-14193 branch from acdfeaf to 674dd0d Compare June 13, 2024 14:26
Copy link
Contributor

❌ Gradle check result for acdfeaf: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

❌ Gradle check result for 674dd0d: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 13, 2024
@kurtmckee
Copy link
Author

kurtmckee commented Jul 13, 2024

It is incorrect to mark this as stalled; this PR exists to ensure that #14193 can be easily tested. Gradle checks are failing because this PR offers the full day of research I put in to make it trivial to demonstrate the "break" part of a "break/fix" PR.

I would like OpenSearch maintainers to add the "fix" commits to this PR to resolve #14193.

@dblock
Copy link
Member

dblock commented Jul 15, 2024

@kurtmckee thank you for opening a PR that demonstrates the problem! Do you know how to fix it? If so please don't wait for someone else to fix it, cc: @navneet1v - are you working on a fix or do you know anyone working on one?

@kurtmckee
Copy link
Author

Unfortunately I'm not familiar with Java at all, and the commit that introduced the issue (ebe6f2c) was too large for me to get a toe-hold into the code. It took almost a full day to produce the "break" commits in this PR, and this is as far as I can go. 😅

@navneet1v
Copy link
Contributor

@dblock I will try to take a look at this. I know where the changes are needed. It might not be a straightforward change as multiple shapes cannot be part of 1 single doc value. There was other ways to do it which was using a GEOMETRY_COLLECTION to achieve this. but give me some time I will try to see the feasibility here.

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jul 16, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Aug 15, 2024
@kurtmckee
Copy link
Author

This PR is not stalled; it is awaiting a OpenSearch developer to push the "fix bug" portion of this "break CI / fix bug" PR. The day I spent to hunt this down and craft this handoff PR should preferably not be discarded by closing this PR.

@dblock
Copy link
Member

dblock commented Aug 15, 2024

This PR is not stalled; it is awaiting a OpenSearch developer to push the "fix bug" portion of this "break CI / fix bug" PR. The day I spent to hunt this down and craft this handoff PR should preferably not be discarded by closing this PR.

Yeah the bot doesn't know that the PR is reproducing a problem, but it's ok even if it gets auto-closed. The issue will remain open and someone who is actually fixing the bug will use it.

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Aug 16, 2024
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Sep 15, 2024
@kurtmckee
Copy link
Author

This PR isn't stalled. I don't want it to be auto-closed.

@navneet1v Have you been able to review this regression?

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Sep 16, 2024
@dblock
Copy link
Member

dblock commented Sep 19, 2024

@kurtmckee Maybe you want to try and bisect the regression to a specific commit? Probably will become clear how it broke and hopefully easy to fix.

@kurtmckee
Copy link
Author

@dblock I spent a full work day doing that several months ago and reported my detailed findings in #14193.

It's commit ebe6f2c, authored by @navneet1v, merged in PRs #4266 and #8301, and it's thousands of lines of changes in a single commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Geospatial Other
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Arrays of geo_shape are broken after v2.8.0
3 participants