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.

Sign up for GitHub

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

Jump to bottom

[Feature] Broker load supports json #31841

Merged
merged 12 commits into from
Nov 10, 2023
Merged

[Feature] Broker load supports json #31841

merged 12 commits into from
Nov 10, 2023

Conversation

rickif
Copy link
Contributor

@rickif rickif commented Sep 26, 2023 •
edited
Loading

Fixes #31435
This PR adds the support of JSON format to broker load. However, the parser needs a completed data payload to parse JSON.
To avoid too much memory usage, a file size limit json_file_size_limit with a default value 4GB is added.

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.2
    • 3.1
    • 3.0
    • 2.5

@mergify mergify bot assigned rickif Sep 26, 2023
@rickif rickif force-pushed the feat/broker-load-supports-json branch 2 times, most recently from cda6cf7 to eecc4ca Compare October 22, 2023 09:17
@rickif rickif force-pushed the feat/broker-load-supports-json branch from 9b5596d to f2389cb Compare October 31, 2023 09:53
@rickif rickif requested a review from a team as a code owner October 31, 2023 09:53
@github-actions github-actions bot deleted a comment from wanpengfei-git Oct 31, 2023
@github-actions github-actions bot deleted a comment from wanpengfei-git Oct 31, 2023
chaoyli
chaoyli reviewed Nov 1, 2023
View reviewed changes
be/src/exec/json_scanner.cpp
if (!res.ok()) {
return res.status();
}
auto sz = res.value();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the sz means the whole file size? If the file size is huge, will it consume much memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The memory usage would be as large as the input size. It's necessary since the json parser we used does not support parsing in streaming mode. To eliminate this effect, the buffer is reused between json files.

@rickif rickif force-pushed the feat/broker-load-supports-json branch 3 times, most recently from 8c97ee9 to fd6adca Compare November 7, 2023 06:47
@rickif rickif force-pushed the feat/broker-load-supports-json branch 2 times, most recently from c4e58a6 to a9f42e4 Compare November 8, 2023 12:16
rickif added 12 commits November 9, 2023 10:00
@rickif
*: broker load supports json
ecbaa7b 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update JsonParserTest
c1fd55f 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update FileScanNodeTest
0753950 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update JsonScannerTest
71c87c3 
Signed-off-by: ricky <rickif@qq.com>
@rickif
fs_hdfs: revert
02e7625 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update JsonScannerTest
94775c6 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update FileScanNodeTest
3f7ea92 
Signed-off-by: ricky <rickif@qq.com>
@rickif
json_scanner: limit the maximum size of json file
e6e4d6d 
Signed-off-by: ricky <rickif@qq.com>
@rickif
config: add json_file_size_limit
18c780f 
Signed-off-by: ricky <rickif@qq.com>
@rickif
*: format
24e7014 
Signed-off-by: ricky <rickif@qq.com>
@rickif
json_scanner: better log
13fdf21 
Signed-off-by: ricky <rickif@qq.com>
@rickif
test: update JsonScannerTest
093202a 
Signed-off-by: ricky <rickif@qq.com>
@rickif rickif force-pushed the feat/broker-load-supports-json branch from a9f42e4 to 093202a Compare November 9, 2023 02:01
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 9, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023

[FE Incremental Coverage Report]

✅ pass : 10 / 10 (100.00%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/planner/FileScanNode.java 6 6 100.00% []
🔵 com/starrocks/common/Config.java 1 1 100.00% []
🔵 com/starrocks/load/BrokerFileGroup.java 2 2 100.00% []
🔵 com/starrocks/planner/StreamLoadScanNode.java 1 1 100.00% []

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 9, 2023

[BE Incremental Coverage Report]

❌ fail : 40 / 54 (74.07%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 be/src/exec/json_scanner.cpp 36 50 72.00% [684, 703, 704, 705, 706, 707, 716, 720, 724, 725, 727, 741, 779, 784]
🔵 be/src/exec/json_parser.cpp 4 4 100.00% []

@meegoo meegoo enabled auto-merge (squash) November 10, 2023 02:43
@meegoo meegoo merged commit 6fb6669 into StarRocks:main Nov 10, 2023
@rickif rickif deleted the feat/broker-load-supports-json branch November 10, 2023 08:04
@rickif
Copy link
Contributor Author

rickif commented Nov 24, 2023

@Mergifyio backport branch-3.2

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 24, 2023 •
edited
Loading

backport branch-3.2

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 24, 2023
@rickif @mergify
[Feature] Broker load supports json (#31841)
ef1e469 
Signed-off-by: ricky <rickif@qq.com>
(cherry picked from commit 6fb6669)

# Conflicts:
#	fe/fe-core/src/main/java/com/starrocks/common/Config.java
@mergify mergify bot mentioned this pull request Nov 24, 2023
Closed
rickif added a commit to rickif/starrocks that referenced this pull request Nov 24, 2023
@rickif
[Feature] Broker load supports json (StarRocks#31841)
61df103 
Signed-off-by: ricky <rickif@qq.com>
@rickif rickif mentioned this pull request Nov 24, 2023
Merged
wyb pushed a commit that referenced this pull request Nov 24, 2023
@rickif
[Feature] Broker load supports json (branch-3.2) (backport #31841 #34881
7b6cd56 
 #35094) (#35739)

Signed-off-by: ricky <rickif@qq.com>
@rickif rickif mentioned this pull request Dec 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meegoo meegoo meegoo approved these changes

@chaoyli chaoyli chaoyli approved these changes

@packy92 packy92 packy92 approved these changes

Assignees

@rickif rickif

Labels
behavior_changed PROTO-REVIEW
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Broker load supports source file in JSON format
4 participants
@rickif @meegoo @chaoyli @packy92