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

enhance: Bulkinsert supports null in csv formats #35912

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

OxalisCu
Copy link
Contributor

@OxalisCu OxalisCu commented Sep 2, 2024

see details in this issue #35911

@sre-ci-robot sre-ci-robot added size/L Denotes a PR that changes 100-499 lines. area/test sig/testing test/integration integration test labels Sep 2, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement ci-passed labels Sep 2, 2024
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

Attention: Patch coverage is 73.46939% with 13 lines in your changes missing coverage. Please review.

Project coverage is 72.61%. Comparing base (b2eb9fe) to head (387f93f).
Report is 59 commits behind head on master.

Files with missing lines Patch % Lines
internal/util/importutilv2/csv/row_parser.go 69.69% 5 Missing and 5 partials ⚠️
internal/util/importutilv2/reader.go 50.00% 1 Missing and 1 partial ⚠️
internal/util/importutilv2/option.go 83.33% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35912      +/-   ##
==========================================
- Coverage   81.60%   72.61%   -9.00%     
==========================================
  Files        1264     1264              
  Lines      150692   150789      +97     
==========================================
- Hits       122976   109488   -13488     
- Misses      22826    36406   +13580     
- Partials     4890     4895       +5     
Files with missing lines Coverage Δ
internal/util/importutilv2/csv/reader.go 71.92% <100.00%> (ø)
internal/util/testutil/test_util.go 93.29% <100.00%> (+0.03%) ⬆️
internal/util/importutilv2/option.go 67.92% <83.33%> (+1.96%) ⬆️
internal/util/importutilv2/reader.go 65.38% <50.00%> (-4.19%) ⬇️
internal/util/importutilv2/csv/row_parser.go 69.66% <69.69%> (-0.10%) ⬇️

... and 239 files with indirect coverage changes


func GetCSVNullKey(options Options) (string, error) {
nullKey, err := funcutil.GetAttrByKeyFromRepeatedKV("nullkey", options)
defaultNullKey := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

seems no need to set a 'defaultNullKey', because reader only try to parse null when nullable. Can simply just return 'NULL' as the null key.

Copy link
Contributor Author

@OxalisCu OxalisCu Sep 5, 2024

Choose a reason for hiding this comment

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

This function is used to parse the nullkey configured by the user. In addition to the defaultNullKey, the user can configure other nullkeys to increase the flexibility of the export format.
For example, the NULL data exported by pg supports configuration http://www.postgres.cn/docs/13/sql-copy.html.
If it is not necessary, I can directly return "NULL".

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is used to parse the nullkey configured by the user. In addition to the defaultNullKey, the user can configure other nullkeys to increase the flexibility of the export format.
For example, the NULL data exported by pg supports configuration http://www.postgres.cn/docs/13/sql-copy.html.
If it is not necessary, I can directly return "NULL".

I see. it's ok for me now to follow the pg support configuration.

@smellthemoon
Copy link
Contributor

/lgtm

@congqixia
Copy link
Contributor

/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: congqixia, OxalisCu

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

The pull request process is described here

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

@sre-ci-robot sre-ci-robot merged commit 3a381bc into milvus-io:master Sep 9, 2024
14 of 16 checks passed
chyezh pushed a commit to chyezh/milvus that referenced this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/test ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm sig/testing size/L Denotes a PR that changes 100-499 lines. test/integration integration test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants