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

vam: type value comparisons #5495

Merged
merged 1 commit into from
Nov 22, 2024
Merged

vam: type value comparisons #5495

merged 1 commit into from
Nov 22, 2024

Conversation

mattnibs
Copy link
Collaborator

No description provided.

@mattnibs mattnibs requested a review from a team November 21, 2024 16:44
Comment on lines 69 to 77
if !(op == "==" || op == "!=") && typ == "TypeValue" {
s += "return vector.NewConst(super.False, lhs.Len(), nil)\n}\n"
return s
}
s += genVarInit("l", typ, lhs)
s += genVarInit("r", typ, rhs)
lexpr := genExpr("l", lhs)
rexpr := genExpr("r", rhs)
if typ == "Bytes" {
if typ == "Bytes" || typ == "TypeValue" {
Copy link
Member

Choose a reason for hiding this comment

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

This should match what happens on the other side of the fence.

case aid == super.IDType:
zctx := super.NewContext() // XXX This is expensive.
// XXX This isn't cheap eventually we should add
// super.CompareTypeValues(a, b zcode.Bytes).
av, _ := zctx.DecodeTypeValue(a.Bytes())
bv, _ := zctx.DecodeTypeValue(b.Bytes())
return super.CompareTypes(av, bv)
.

And since performance on type comparisons doesn't matter much, I don't think it's worth generating code and suggest we just handle it with a loop over in compare.go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator Author

@mattnibs mattnibs Nov 21, 2024

Choose a reason for hiding this comment

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

You are referencing sort, for equality comparisons on type values we just compare bytes:

switch aid, bid := a.Type().ID(), b.Type().ID(); {
case !super.IsNumber(aid) || !super.IsNumber(bid):
return aid == bid && bytes.Equal(a.Bytes(), b.Bytes())

Copy link
Member

Choose a reason for hiding this comment

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

Oh, boy. We do different things depending on whether the right-hand side of the comparison is constant.

$ echo '{i32:<int32>,i64:<int64>}' | super -c 'yield i32 < i64, i32 < <int64>' -
op <
false
true

We should fix that but obviously not here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mccanne has proposed dropping the comparison constant optimization for sam which I'm beginning to agree with.

@mattnibs mattnibs force-pushed the vam-compare-typevals branch 5 times, most recently from a04cccd to e090566 Compare November 21, 2024 21:16
@mattnibs mattnibs merged commit a6e7be7 into main Nov 22, 2024
3 checks passed
@mattnibs mattnibs deleted the vam-compare-typevals branch November 22, 2024 16:49
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