SteNicholas commented on code in PR #181:
URL: https://github.com/apache/paimon-cpp/pull/181#discussion_r3725580008
##########
cmake_modules/SetupCxxFlags.cmake:
##########
@@ -22,6 +22,30 @@ include(CheckCXXCompilerFlag)
message(STATUS "System processor: ${CMAKE_SYSTEM_PROCESSOR}")
+include("${CMAKE_CURRENT_LIST_DIR}/TargetArchitecture.cmake")
+message(STATUS "Target processor: ${PAIMON_TARGET_PROCESSOR} "
+ "(CPU family: ${PAIMON_TARGET_CPU_FAMILY})")
+
+# Probe the architecture specific tuning flags. x86 deliberately gets none of
its
+# own, so there is nothing to probe there: the baseline stays whatever the
+# toolchain defaults to, and no persisted value depends on it; see
+# src/paimon/common/utils/crc32c.h.
+if(PAIMON_TARGET_CPU_FAMILY STREQUAL "ppc")
Review Comment:
Good point, done — the updated revision drops the ppc pieces altogether: the
`-maltivec` probe and the ppc block in `SetupCxxFlags.cmake` are removed, and
`TargetArchitecture.cmake` has no ppc mapping, so a ppc host resolves to the
`unknown` family and keeps configuring with toolchain defaults, exactly as it
effectively did before. `cmake_modules/tests/target_architecture_test.cmake`
pins that ("ppc64le is not an error"), and the commit message records the
rationale.
##########
src/paimon/common/data/binary_string.cpp:
##########
@@ -75,16 +76,19 @@ std::string BinaryString::ToString() const {
}
int32_t BinaryString::NumBytesForFirstByte(char b) {
Review Comment:
Fixed in the updated revision, all three spots:
- The pre-check `(*bytes)[0] = tolower(...)` lines are removed outright. As
you note they ran before the ASCII check, and they used `tolower` even in
`ToUpperCase`; the loop writes every byte including index 0, so they were also
redundant.
- The loop now calls `toupper`/`tolower` through `static_cast<unsigned
char>(b)`, so a byte at or above 0x80 reaches the ctype functions as a
non-negative value.
- `CppToUpperCase`/`CppToLowerCase` pass a lambda taking `unsigned char` to
`std::transform` instead of `::toupper`/`::tolower` directly.
`binary_string_test.cpp` now asserts that invalid UTF-8 bytes (`0x80`,
`0xff`) pass through case conversion unchanged on both char-signedness ABIs.
##########
src/paimon/core/casting/numeric_primitive_cast_executor.h:
##########
@@ -46,14 +50,69 @@ class NumericPrimitiveCastExecutor : public CastExecutor {
const
std::shared_ptr<arrow::DataType>& target_type,
arrow::MemoryPool* pool) const
override;
+ /// Converts a floating point value to an integer type the way Java does,
where a plain
Review Comment:
Done — `read.rst` is updated in this revision: note 2️⃣ under the Type
Change Support Matrix now records the Java-consistent semantics (`NaN` becomes
0, an out-of-range value saturates at the int32 bounds — int64 for `bigint` —
and is then narrowed to the target width; float -> tinyint: `MAX_FLOAT -> -1`,
`INFINITY -> -1`, `NaN -> 0`, `300.9 -> 44`), and the overflow section above
the matrix calls out float/double -> integer as the one case that is now well
defined and matches Java on every supported architecture.
##########
build_support/asan_symbolize.py:
##########
@@ -333,8 +334,17 @@ def process_stdin(self):
if sys.version_info[0] == 2:
sys.stdout = os.fdopen(sys.stdout.fileno(), 'w', 0)
else:
- # Unbuffered output is not supported in Python 3
- sys.stdout = os.fdopen(sys.stdout.fileno(), 'w')
+ # Test output is not guaranteed to be valid UTF-8, so pin both streams
to the same
+ # encoding and round such bytes through surrogateescape, which passes
them out
+ # unchanged instead of raising and truncating the rest of the test log.
Both the
+ # encoding and the error handler have to be set on both streams: taking
either from
+ # the locale or from PYTHONIOENCODING would let them disagree and
re-encode the bytes.
+ # The buffers are rewrapped rather than reconfigured, which needs Python
3.7, and
+ # line buffering stands in for the unbuffered output Python 3 does not
support.
+ sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8',
Review Comment:
This file is vendored third-party code (LLVM compiler-rt's
`asan_symbolize.py`, via Apache Arrow), so per ASF policy it keeps its upstream
license header and must not gain an ASF one — local modifications do not change
that: https://www.apache.org/legal/src-headers.html#3party. It is accounted for
in `LICENSE` ("This product includes code from LLVM compiler-rt", pointing at
`licenses/LICENSE-compiler-rt.txt`) and excluded from the RAT check via
`.github/.rat-excludes`, so `rat-license-check` stays green as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
