[PECO-1080] Added version check that throws on low protocol version #181
Conversation
nithinkdb
requested review from
andrefurlan-db,
arikfr,
kravets-levko,
superdupershant,
susodapop and
yunbodeng-db
as code owners
nithinkdb
had a problem deploying
to
azure-prod
GitHub Actions
Failure
susodapop
left a comment
susodapop
left a comment
There was a problem hiding this comment.
Approving for now. Are there tests for parameterized queries? If so, do we need to update the e2e test since getQueryParameters has a different function signature? Any other usages of getQueryParameters that we should consider?
kravets-levko
left a comment
kravets-levko
left a comment
There was a problem hiding this comment.
LGTM. Should we add this to v1.5.0? If no - then please don't merge until we cut a release
|
@susodapop |
| result.push(sparkParam); | ||
| if ( | ||
| sessionHandle?.serverProtocolVersion && | ||
| sessionHandle.serverProtocolVersion >= TProtocolVersion.SPARK_CLI_SERVICE_PROTOCOL_V8 |
There was a problem hiding this comment.
Actually, one more thing I juts realized - when we merged parameters PR, the current protocol version at that moment was SPARK_CLI_SERVICE_PROTOCOL_V6, and the feature worked. Maybe we can be less strict here?
There was a problem hiding this comment.
Nope, there were no server side checks for protocol, which is why the protocol didn't matter before. But Yunbo has said that we shouldn't enable parameterized queries on anything past V8, and we've actually prevented parameters from being passed on the server side unless the protocol version is V8.
3 participants
Currently, the server side will not enable parameterized queries unless a flag manually overrides, or the client protocol/server protocol are both v8. However, if it detects a lower version, it won’t return an error; it will just pass an empty set of parameters. We need to add a client side check to ensure more transparency for the user.