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

fix(query): pushdown joins only if lhs/rhs shard keys are identical #1513

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

alextheimer
Copy link
Contributor

Pull Request checklist

  • The commit(s) message(s) follows the contribution guidelines ?
  • Tests for the changes have been added (for bug fixes / features) ?
  • Docs have been added / updated (for bug fixes / features) ?

Currently, queries such as:

// target-schema labels:  shard_key, label1
foo{shard_key="aaa"} + on(label1) bar{shard_key="bbb"}
// Note: shard-key labels are implicit in the on() clause.

will be pushed down because planner logic only requires that the join operands have target-schemas defined against the same set of labels.

This PR adds logic to also require that shard keys are identical on each side of the join.

case _ => None
}
val tsFunc = tsp.targetSchemaFunc(filters.head)
startEndMsPairOpt match {
Copy link
Contributor

Choose a reason for hiding this comment

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

startEndMsPairOpt.map() is cleaner than pattern match that return None for None

case _ => None
}
}
val tsLabels: Option[Seq[String]] = getTargetSchemaLabelsFromRawSeries(rs, qContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a method? This lead to code duplication and doing map instead of pattern match will be concise as well

val rhsData = helper(bj.rhs)
val canPushdown = lhsData.isDefined && rhsData.isDefined && {
val hasScalar = lhsData.get.shards.isEmpty || rhsData.get.shards.isEmpty
val canPushdownJoin = lhsData.get.shards == rhsData.get.shards &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the order of Seq containing shards granted to be ordered consistently? Seq(1, 2, 3) != Seq(3, 2, 1) though you want to push down.

Also too much going on here, some comments would really be helpful to understand the intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants