Skip to content

Conversation

@kaka11chen
Copy link
Contributor

Proposed changes

After supporting insert-only transactional hive full acid tables #19518, #19419, this PR support transactional hive full acid tables.

  • Support hive3 transactional hive full acid tables.
  • Hive2 transactional hive full acid tables need to run major compactions.

Further comments

Regression test only test with hive2 transactional hive full acid tables now, because the docker image version is hive 2.x, will add hive 3.x. docker later.

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@kaka11chen
Copy link
Contributor Author

run buildall

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@kaka11chen
Copy link
Contributor Author

run buildall

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

if (columnsFromPath == null || columnsFromPath.isEmpty()) {
return Collections.emptyList();
}
int pathCount = isACID ? 3 : 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment explain 3 and 2, better give an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

}
}

private List<Split> splitFile(Path path, long blockSize, BlockLocation[] blockLocations, long length,
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to find a way to extract the common part of splitFile and the same method defined in FileScanNode, or it is hard to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@kaka11chen
Copy link
Contributor Author

run buildall

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Jun 12, 2023
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@kaka11chen
Copy link
Contributor Author

run buildall

@yiguolei yiguolei merged commit 73ad885 into apache:master Jun 13, 2023
AshinGau pushed a commit that referenced this pull request Jun 16, 2023
…ding hive-docker missing configurations. (#20832)

Fix hive transaction table regression test test_transactional_hive by adding hive-docker missing configurations of #20679. Hive need to be set these configurations to do compaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. area/planner Issues or PRs related to the query planner kind/test reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants