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

executor: handle index join for the new partition table implementation #18862

Merged
merged 9 commits into from
Aug 5, 2020

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

In #18823, the three readers is translated to partition table.
But for index join, that is not sufficient, because then inner table is constructed dynamically during runtime.

What is changed and how it works?

What's Changed:

Handle partition table in dataReaderBuilder, build partition table for index join.

How it Works:

  • Update buildIndexReaderForIndexJoin
  • Update buildIndexLookUpReaderForIndexJoin
  • Introduce buildRangesForIndexJoin function, the dataReader should not build kvRange because the table ID may change.

In buildExecutorForIndexJoinInternal, there are some other cases, I'm not sure when the code run there:

	switch v := plan.(type) {
	case *plannercore.PhysicalTableReader:
	case *plannercore.PhysicalIndexReader:
	case *plannercore.PhysicalIndexLookUpReader:
	case *plannercore.PhysicalUnionScan:
	case *plannercore.PhysicalProjection:
	case *plannercore.PhysicalSelection:
	case *mockPhysicalIndexReader:
	}

Related changes

Check List

Tests

  • Unit test

Release note

  • No release note

This change is based on #18823, that PR should be merged first.

@tiancaiamao tiancaiamao requested review from a team as code owners July 29, 2020 08:26
@tiancaiamao tiancaiamao requested review from wshwsh12 and winoros and removed request for a team July 29, 2020 08:26
@tiancaiamao tiancaiamao added the sig/execution SIG execution label Jul 29, 2020
@tiancaiamao
Copy link
Contributor Author

PTAL @imtbkcat @lysu @winoros

@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #18862 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #18862   +/-   ##
===========================================
  Coverage   79.4730%   79.4730%           
===========================================
  Files           546        546           
  Lines        148220     148220           
===========================================
  Hits         117795     117795           
  Misses        20948      20948           
  Partials       9477       9477           

executor/builder.go Outdated Show resolved Hide resolved
}

nextPartition := nextPartitionForIndexLookUp{exec: ret}
exec, err := buildPartitionTable(b, ts.Table, v.PruningConds, ret, nextPartition)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe have same question, when change test where p.id = t.id to where p.c = t.id

Copy link
Contributor Author

@tiancaiamao tiancaiamao Jul 30, 2020

Choose a reason for hiding this comment

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

It's a bit more difficult because the the reader reads chunk by chunk. Inside a chunk, each row may correspond to diffierent table.
The current TableReader/IndexReader/IndexLookUp have an assumption that the underlying table is fixed and not change, so the build key range logic need update in those three reader executor.

I plan to improve here later, not in this PR.
Although the partition pruning is not done for some case, the code should be able to run.

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 3, 2020
@tiancaiamao
Copy link
Contributor Author

/rebuild

@tiancaiamao
Copy link
Contributor Author

/rebuild

rpc error: code = Unknown desc = can not get timestamp, may be not leader ("rpc error: code = Unknow
n desc = can not get timestamp, may be not leader")

Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 5, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 5, 2020
@tiancaiamao
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 5, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants