Skip to content

fix: prevent crash when ARRAY_CONTAINS_ALL is used with empty array#41347

Closed
Hoyaspark wants to merge 4 commits intomilvus-io:masterfrom
Hoyaspark:fix/json-contains-empty-array-crash
Closed

fix: prevent crash when ARRAY_CONTAINS_ALL is used with empty array#41347
Hoyaspark wants to merge 4 commits intomilvus-io:masterfrom
Hoyaspark:fix/json-contains-empty-array-crash

Conversation

@Hoyaspark
Copy link
Contributor

@Hoyaspark Hoyaspark commented Apr 16, 2025

What this PR does

  • Fixes a crash that occurs when using like ARRAY_CONTAINS_ALL... with an empty array in a filter expression.

How it works

This PR adds a guard clause in PhyJsonContainsFilterExpr::EvalJsonContainsForDataSegment() that:

  • Checks whether expr_->vals_ is empty
  • Checks whether val_case() is set on the first value

If not, it raises a PanicInfo(DataTypeInvalid) with a clear error message.

related issue : #41348

Signed-off-by: Sangho Park <hoyaspark@gmail.com>
@sre-ci-robot sre-ci-robot added the size/XS Denotes a PR that changes 0-9 lines. label Apr 16, 2025
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Hoyaspark
To complete the pull request process, please assign liliu-z after the PR has been reviewed.
You can assign the PR to them by writing /assign @liliu-z in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Apr 16, 2025
@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

@Hoyaspark Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

@codecov
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.87%. Comparing base (afb1b85) to head (ef32a89).
Report is 235 commits behind head on master.

Current head ef32a89 differs from pull request most recent head 90bc4bb

Please upload reports for the commit 90bc4bb to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #41347       +/-   ##
===========================================
+ Coverage   72.32%   81.87%    +9.54%     
===========================================
  Files         310     1166      +856     
  Lines       28670   181134   +152464     
===========================================
+ Hits        20736   148302   +127566     
- Misses       7934    26692    +18758     
- Partials        0     6140     +6140     
Components Coverage Δ
Client 79.59% <ø> (∅)
Core ∅ <ø> (∅)
Go 81.98% <ø> (∅)

see 1476 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

@Hoyaspark E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

…pr values

Signed-off-by: Sangho Park <hoyaspark@gmail.com>
…Error

Signed-off-by: Sangho Park <hoyaspark@gmail.com>
@sre-ci-robot sre-ci-robot added size/M Denotes a PR that changes 30-99 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Apr 16, 2025
@Hoyaspark
Copy link
Contributor Author

Replaced the panic in JsonContainsExpr with a SegcoreError exception when empty values are provided, and added a corresponding test case to verify the behavior.

Let me know if you have any suggestions or better ideas on how to handle this.

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

@Hoyaspark cpp-unit-test check failed, comment rerun cpp-unit-test can trigger the job again.

Signed-off-by: Sangho Park <hoyaspark@gmail.com>
@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

@Hoyaspark go-sdk check failed, comment rerun go-sdk can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Apr 16, 2025

@Hoyaspark cpp-unit-test check failed, comment rerun cpp-unit-test can trigger the job again.

czs007 pushed a commit that referenced this pull request May 14, 2025
…1831)

issue: #41348

related and optimized by #41347

master pr: #41739
2.5 pr: #41756

Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
Co-authored-by: Sangho Park <hoyaspark@gmail.com>
sre-ci-robot pushed a commit that referenced this pull request May 14, 2025
#41756)

issue: #41348

related and optimized by #41347

master pr: #41739

---------

Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
Co-authored-by: Sangho Park <hoyaspark@gmail.com>
sre-ci-robot pushed a commit that referenced this pull request May 14, 2025
…1739)

issue: #41348 

related and optimized by #41347

---------

Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
Co-authored-by: Sangho Park <hoyaspark@gmail.com>
@stale
Copy link

stale bot commented May 16, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label May 16, 2025
nish112022 pushed a commit to nish112022/milvus that referenced this pull request May 22, 2025
milvus-io#41756)

issue: milvus-io#41348

related and optimized by milvus-io#41347

master pr: milvus-io#41739

---------

Signed-off-by: Cai Zhang <cai.zhang@zilliz.com>
Co-authored-by: Sangho Park <hoyaspark@gmail.com>
@stale stale bot closed this May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-passed DCO check passed. kind/bug Issues or changes related a bug size/M Denotes a PR that changes 30-99 lines. stale indicates no udpates for 30 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants