Support 32-bit architecture in stream
Summary
The PR introduces two improvements related to the stream utility:
- Replaces the hardcoded value with the
MAX_STRING_LENGTHconstant 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. - Replaces the use of
anywithunknownin theisStreamfunction.
I didn't add tests since the main flow remains unchanged, and the update addresses 32-bit architectures only.
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 https://github.com/ClickHouse/clickhouse-js/pull/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.
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.