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

[BugFix] Fix hang because of empty scan ranges #9458

Merged
merged 1 commit into from
Aug 3, 2022
Merged

[BugFix] Fix hang because of empty scan ranges #9458

merged 1 commit into from
Aug 3, 2022

Conversation

dirtysalt
Copy link
Contributor

@dirtysalt dirtysalt commented Aug 2, 2022

What type of PR is this:

  • bug
  • feature
  • enhancement
  • refactor
  • others

Which issues of this PR fixes :

Fixes #https://github.com/StarRocks/StarRocksTest/issues/870

Fixes #9411

Problem Summary(Required) :

We don't handle empty scan range properly.

Copy link
Contributor

@imay imay left a comment

Choose a reason for hiding this comment

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

LGTM

return Status::OK();
}

Status ConnectorScanNode::_start_scan_thread(RuntimeState* state) {
for (TScanRangeParams& scan_range : _scan_ranges) {
_create_and_init_scanner(state, scan_range.scan_range);
}
_num_scanners = _pending_scanners.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

when will _num_scanners == 0 happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DorianZheng when there is no scan file in this table.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it. Why not prevent this case on FE?

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. That would also solve the problem. It generates an EMPTY NODE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it generate an empty node instead of return early?

Copy link
Contributor

@DorianZheng DorianZheng left a comment

Choose a reason for hiding this comment

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

left a comment, rest LGTM

@wanpengfei-git wanpengfei-git added the Approved Ready to merge label Aug 2, 2022
@wanpengfei-git
Copy link
Collaborator

run starrocks_admit_test

@dirtysalt dirtysalt merged commit 9f538f4 into StarRocks:main Aug 3, 2022
@dirtysalt dirtysalt deleted the fix-empty-scan-ranges branch August 9, 2022 07:22
@dirtysalt
Copy link
Contributor Author

@Mergifyio backport branch-2.3

mergify bot pushed a commit that referenced this pull request Aug 17, 2022
(cherry picked from commit 9f538f4)

# Conflicts:
#	be/src/exec/vectorized/connector_scan_node.cpp
@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2022

backport branch-2.3

✅ Backports have been created

@dirtysalt
Copy link
Contributor Author

@Mergifyio backport branch-2.4

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2022

backport branch-2.4

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 17, 2022
(cherry picked from commit 9f538f4)

# Conflicts:
#	be/test/formats/parquet/file_reader_test.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Insert data from the Hive external table into the SR internal table is abnormal 【SR 2.3】
5 participants