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

sql: propagate default int size to remote nodes #50035

Closed
wants to merge 1 commit into from

Conversation

yuzefovich
Copy link
Member

A while ago we added support of DEFAULT_INT_SIZE which can be set
either to 8 (default) or 4 bytes. So far that setting has been used to
influence how we parse the "naked" INT in the query from the client.
However, there is one other place where we're performing the parsing:
during expression deserialization on the remote nodes. Previously, the
setting hasn't been propagated to remote nodes, so we might have
interpreted it incorrectly. This commit addresses that.

Release note: None

A while ago we added support of `DEFAULT_INT_SIZE` which can be set
either to 8 (default) or 4 bytes. So far that setting has been used to
influence how we parse the "naked" INT in the query from the client.
However, there is one other place where we're performing the parsing:
during expression deserialization on the remote nodes. Previously, the
setting hasn't been propagated to remote nodes, so we might have
interpreted it incorrectly. This commit addresses that.

Release note: None
@yuzefovich yuzefovich added the do-not-merge bors won't merge a PR with this label. label Jun 10, 2020
@yuzefovich yuzefovich requested review from jordanlewis and knz June 10, 2020 03:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member Author

Together with @jordanlewis we found this when looked into some test failures while I was working on #49891. However, now I believe that this propagation is actually unnecessary - my current understanding is that we see "naked" INT type only when it's coming from the client, and we parse it into either INT8 or INT4. From that point on forward we don't have INTs anywhere, including the serialized representation. Is my understanding correct? If so, I think this PR attempts to fix a problem that doesn't exist and I'll close it.

@knz
Copy link
Contributor

knz commented Jun 10, 2020 via email

@yuzefovich
Copy link
Member Author

I think that it's useful for consistency, so that there's no discrepancy between local and remote execution.

Hm, did you mean that we should propagate an update to default_int_size setting throughout the whole cluster (which I don't think we currently do)? Or that DistSQL execution should propagate it as part of the eval context proto (which is what this PR does)?

@knz
Copy link
Contributor

knz commented Jun 11, 2020

I mean what this PR does. I was surprised it doesn't already actually; I thought the entire SessionData struct was propagated as-is.

Maybe it could be an idea to make SessionData a protobuf and have the same struct used directly by the executor and serialized with the distsql plans. This way it'd always be up to date.

@yuzefovich
Copy link
Member Author

Maybe it could be an idea to make SessionData a protobuf and have the same struct used directly by the executor and serialized with the distsql plans. This way it'd always be up to date.

It's an interesting idea. I think there might be a possible downside to it: it seems that many packages import sessiondata package which currently has just a couple of dependencies, but if we make SessionData struct a protobuf, then we might be bringing in more dependencies in other packages, possibly making build times slower in some scenarios. Does this sound plausible? I'm not very familiar with build process in general, so maybe I'm making stuff up.

Anyway, I'll bring this idea up during our weekly meeting tomorrow.

@asubiotto
Copy link
Contributor

@yuzefovich to create an issue

@knz
Copy link
Contributor

knz commented Sep 19, 2020

I think this is still relevant, especially in light of the recent realization that we get internal executors spawned on distsql proecssors. Casts executed remotely need to know about the int size!!

@yuzefovich
Copy link
Member Author

#55569 does more than this PR, so closing.

@yuzefovich yuzefovich closed this Oct 16, 2020
@yuzefovich yuzefovich deleted the default-int-size branch October 16, 2020 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge bors won't merge a PR with this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants