exec: handle some mixed type comparison expressions #39464

rafiss · 2019-08-08T17:35:38Z

This adds support for int-int comparisons and float-float comparisons
(of different sizes), as well as various string comparisons. This
is necessary to support TPC-H queries 7, 16, 19, and 21 (though there
are still other issues to address before those are fully supported).

There is some refactoring here so that as a next step we can support
comparisons between different types entirely (e.g. int-decimal).

touches #39189

Release note: None

cockroach-teamcity · 2019-08-08T17:35:45Z

This change is

rohany

Reviewed 7 of 16 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @rohany)

pkg/sql/distsqlrun/column_exec_setup.go, line 911 at r1 (raw file):

		// handles invalid types for the IN and NOT IN operations at this point,
		// and we allow a comparison between t and t tuple.
		if t.Operator == tree.In || t.Operator == tree.NotIn {

I don't think we should remove this? I don't think the same rules apply for IN/NOT IN

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 373 at r1 (raw file):

}

func (c intCustomizer) getCmpOpCompareFunc() compareFunc {

is the idea that we add more functions like this + compatible types below to add support for mixed type operations?

pkg/sql/exec/compare.go, line 16 at r1 (raw file):

// compareInts compares two int values. This function allows us to easily
// handle mixec-type integer comparison.

[nit] misspelled

yuzefovich

Nice work! A few nits from me.

Reviewed 9 of 16 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

pkg/sql/distsqlrun/column_exec_setup.go, line 849 at r1 (raw file):

	}
	if rConstArg, rConst := right.(tree.Datum); rConst {

[nit]: extra newline.

pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 221 at r1 (raw file):

{{range $left := .ConstSides}}
// GetProjectionConstOperator returns the appropriate constant projection
// operator for the given column type and comparison.

[nit]: this comment needs minor update as well.

pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 289 at r1 (raw file):

// GetProjectionOperator returns the appropriate projection operator for the
// given column type and comparison.

ditto

pkg/sql/exec/execgen/cmd/execgen/selection_ops_gen.go, line 253 at r1 (raw file):

	typToOverloads := make(map[types.T][]*overload)
	for _, overload := range comparisonOpOverloads {
		if overload.LTyp != overload.RTyp {

[nit]: a comment why we're skipping mixed types here might be helpful.

pkg/sql/exec/types/types.go, line 66 at r1 (raw file):

	CompatibleTypes[Bytes] = append(CompatibleTypes[Bytes], Bytes)
	CompatibleTypes[Decimal] = append(CompatibleTypes[Decimal], Decimal)
	CompatibleTypes[Int8] = append(CompatibleTypes[Int8], Int8)

[nit]: it might be cleaner to define "all int types" and use that to append to compatible types.

pkg/sql/logictest/testdata/logic_test/vectorize, line 70 at r1 (raw file):

# Mixed type comparison
query IB
SELECT c, c > 1 FROM a LIMIT 3

[nit]: I think you might be able to cast an INT8 to INT4 here to make it explicit mixed-type comparison (then you'd also not need a new column). Additionally, it'd be nice to see comparison with other int types.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/sql/distsqlrun/column_exec_setup.go, line 849 at r1 (raw file):

Previously, yuzefovich wrote…

[nit]: extra newline.

done

pkg/sql/distsqlrun/column_exec_setup.go, line 911 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I don't think we should remove this? I don't think the same rules apply for IN/NOT IN

Not sure I follow -- previously we already were returning a nil error here in the case of IN/NOT IN. We will continue to do that.

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 373 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

is the idea that we add more functions like this + compatible types below to add support for mixed type operations?

yeah, that was my thinking. though to really support mixed types, we'd need to have customizers that are like intDecimal (etc), not just between different ints.

pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 221 at r1 (raw file):

Previously, yuzefovich wrote…

[nit]: this comment needs minor update as well.

done

pkg/sql/exec/execgen/cmd/execgen/projection_ops_gen.go, line 289 at r1 (raw file):

Previously, yuzefovich wrote…

ditto

done

pkg/sql/exec/execgen/cmd/execgen/selection_ops_gen.go, line 253 at r1 (raw file):

Previously, yuzefovich wrote…

[nit]: a comment why we're skipping mixed types here might be helpful.

actually, on further consideration, we should support mixed types here too

pkg/sql/exec/compare.go, line 16 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

[nit] misspelled

done

rohany

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/sql/distsqlrun/column_exec_setup.go, line 911 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

Not sure I follow -- previously we already were returning a nil error here in the case of IN/NOT IN. We will continue to do that.

Yeah never mind, I think I misunderstood.

yuzefovich

Reviewed 14 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/col/coltypes/types.go, line 66 at r2 (raw file):

	CompatibleTypes[Bytes] = append(CompatibleTypes[Bytes], Bytes)
	CompatibleTypes[Decimal] = append(CompatibleTypes[Decimal], Decimal)
	CompatibleTypes[Int8] = append(CompatibleTypes[Int8], Int8)

[nit]: it might be cleaner to define "all int types" and use that to append to compatible types.

pkg/sql/distsqlrun/column_exec_setup.go, line 915 at r2 (raw file):

		}

		if !left.Identical(right) {

I think this part needs to be kept though, right?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/sql/distsqlrun/column_exec_setup.go, line 915 at r2 (raw file):

Previously, yuzefovich wrote…

I think this part needs to be kept though, right?

This part is removed because we will now allow comparisons to happen even if the left and right type are not identical.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/col/coltypes/types.go, line 66 at r2 (raw file):

Previously, yuzefovich wrote…

[nit]: it might be cleaner to define "all int types" and use that to append to compatible types.

yeah i like that idea. i'll do it

rafiss · 2019-08-15T14:17:25Z

This is ready for another review, whenever someone has a chance

yuzefovich

but I haven't thought much about mixed-type comparisons, so ideally I'd wait for Rohan's approval.

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

pkg/sql/distsqlrun/column_exec_setup.go, line 915 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

This part is removed because we will now allow comparisons to happen even if the left and right type are not identical.

Ok, SGTM.

rohany

This LGTM, should probably work with matt next to get tests that stress the mixed type operations here.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @yuzefovich)

This adds support for int-int comparisons and float-float comparisons (of different sizes), as well as various string comparisons. This is necessary to support TPC-H queries 7, 16, 19, and 21 (though there are still other issues to address before those are fully supported). There is some refactoring here so that as a next step we can support comparisons between different types entirely (e.g. int-decimal). touches cockroachdb#39189 Release note: None

rafiss · 2019-08-16T15:22:45Z

thanks all!

bors r+

39464: exec: handle some mixed type comparison expressions r=rafiss a=rafiss This adds support for int-int comparisons and float-float comparisons (of different sizes), as well as various string comparisons. This is necessary to support TPC-H queries 7, 16, 19, and 21 (though there are still other issues to address before those are fully supported). There is some refactoring here so that as a next step we can support comparisons between different types entirely (e.g. int-decimal). touches #39189 Release note: None 39472: opt: prune RHS of a semi/anti join r=ridwanmsharif a=ridwanmsharif Fixes #38704. This change adds a PruneSemiAntiJoinRightCols to prune columns in the RHS of a semi/anti join. Alternatively, we could just tag the PruneJoinRightCols rule as higher priority to achieve the same effect (previously that rule was never triggered because the `EliminateProjects` rule would fire and remove the projections after `PruneJoinLeftCols` rule is applied). I prefer this rule because it avoids requiring an ordering of transformations. Release note: None Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Ridwan Sharif <[email protected]>

craig · 2019-08-16T15:49:30Z

Build succeeded

GitHub CI (Cockroach)

rafiss requested review from jordanlewis, rohany and a team August 8, 2019 17:35

rohany reviewed Aug 8, 2019

View reviewed changes

yuzefovich reviewed Aug 12, 2019

View reviewed changes

rafiss commented Aug 14, 2019

View reviewed changes

rohany reviewed Aug 14, 2019

View reviewed changes

rafiss force-pushed the mixed-type-comparison-overloads branch from 446730d to 6c6395c Compare August 14, 2019 18:00

rafiss requested a review from a team August 14, 2019 18:11

yuzefovich reviewed Aug 14, 2019

View reviewed changes

rafiss commented Aug 14, 2019

View reviewed changes

rafiss force-pushed the mixed-type-comparison-overloads branch from 6c6395c to a1a1024 Compare August 15, 2019 14:16

rafiss requested review from a team and yuzefovich August 15, 2019 14:16

yuzefovich approved these changes Aug 15, 2019

View reviewed changes

jordanlewis mentioned this pull request Aug 15, 2019

exec: tracking issue for TPC-H queries #34497

Closed

37 tasks

rohany approved these changes Aug 15, 2019

View reviewed changes

rafiss force-pushed the mixed-type-comparison-overloads branch from a1a1024 to 5ecb366 Compare August 15, 2019 17:24

rafiss force-pushed the mixed-type-comparison-overloads branch from 5ecb366 to bc09926 Compare August 15, 2019 18:23

craig bot merged commit bc09926 into cockroachdb:master Aug 16, 2019

rafiss deleted the mixed-type-comparison-overloads branch August 19, 2019 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exec: handle some mixed type comparison expressions #39464

exec: handle some mixed type comparison expressions #39464

rafiss commented Aug 8, 2019

cockroach-teamcity commented Aug 8, 2019

rohany left a comment

yuzefovich left a comment

rafiss left a comment

rohany left a comment

yuzefovich left a comment

rafiss left a comment

rafiss left a comment

rafiss commented Aug 15, 2019

yuzefovich left a comment

rohany left a comment

rafiss commented Aug 16, 2019

craig bot commented Aug 16, 2019

exec: handle some mixed type comparison expressions #39464

exec: handle some mixed type comparison expressions #39464

Conversation

rafiss commented Aug 8, 2019

cockroach-teamcity commented Aug 8, 2019

rohany left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Aug 15, 2019

yuzefovich left a comment

Choose a reason for hiding this comment

rohany left a comment

Choose a reason for hiding this comment

rafiss commented Aug 16, 2019

craig bot commented Aug 16, 2019

Build succeeded