Skip to content

Conversation

@shevchenkonik
Copy link
Contributor

Summary

The PR introduces two improvements related to the stream utility:

  1. Replaces the hardcoded value with the MAX_STRING_LENGTH constant from node.buffer. The issue with the hardcoded value is that it doesn't account for 32-bit architectures and the function couldn't detect the limit.
  2. Replaces the use of any with unknown in the isStream function.

I didn't add tests since the main flow remains unchanged, and the update addresses 32-bit architectures only.

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2025

CLA assistant check)
All committers have signed the CLA.

@slvrtrn
Copy link
Contributor

slvrtrn commented Mar 28, 2025

Thanks for the contribution.

I don't think we can simply use constants because previously, when the "max string length" value was unbound, it was throwing a cryptic error that was difficult for the end user to understand, and that happened even on a 64-bit system: see #357; that's why it is limited to exactly that value.

Perhaps we can detect that the client runs on a 32-bit system and use another value, which is probably also lower than the value provided in the buffer constants.

@shevchenkonik
Copy link
Contributor Author

Hello @slvrtrn, thank you for the answer.

The introduced variable into clickhouse-js equals to 536870888 and it works perfectly with 64-bit architectures. Unfortunately, this value is higher than the 32-bit architecture supports in terms of the v8 engine. V8 engine value for 32-bit is 268435440. It means that we can't catch issue by this condition when the codebase is running into 32-bit.

I have checked node versions from 16 (because package.json says this library supports only ^16), and all major node versions have the MAX_STRING_LENGTH variable equal to 536870888.

The changes in this PR haven't changed the flow of the function, and it should catch this error as before. However, using variables from node helps us catch the issue for both use cases, not only for 64-bit.

@slvrtrn slvrtrn changed the base branch from main to 1.11.1 April 10, 2025 15:24
@slvrtrn slvrtrn merged commit 0fe12b5 into ClickHouse:1.11.1 Apr 10, 2025
22 of 26 checks passed
@slvrtrn slvrtrn mentioned this pull request Apr 10, 2025
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.

3 participants