github-actions[bot] commented on code in PR #67949:
URL: https://github.com/apache/doris/pull/67949#discussion_r4004408848
##########
be/src/core/data_type_serde/data_type_string_serde.cpp:
##########
@@ -847,12 +847,15 @@ Status
DataTypeStringSerDeBase<ColumnType>::from_string(StringRef& str, IColumn&
template <typename ColumnType>
Status DataTypeStringSerDeBase<ColumnType>::from_olap_string(const
std::string& str, Field& field,
const
FormatOptions& options) const {
- // CHAR(N) writes through OlapColumnDataConvertorChar are zero-padded to
- // the declared schema length, so the serialized OLAP string carries
- // trailing '\0' bytes. strnlen() drops that padding to surface the
- // logical character content in the Field. VARCHAR / STRING never write
- // trailing '\0' through this path, so strnlen is a no-op for them.
- size_t len = strnlen(str.data(), str.size());
+ // CHAR(N) is zero-padded to the declared schema length before it is
written, so its
+ // stored bytes carry trailing '\0' and stop at the first one. The page
read path cuts
+ // CHAR values the same way (see BinaryPlainPageCharStripPreDecoder), so a
bound built
+ // like this stays comparable with the rows it describes.
+ //
+ // VARCHAR and STRING keep every byte they were given, '\0' included.
Cutting such a
+ // value at an embedded '\0' would give a bound the data never held, and a
zone map
+ // built from it prunes rows that match.
+ size_t len = _type == TYPE_CHAR ? strnlen(str.data(), str.size()) :
str.size();
Review Comment:
[P1] Handle saturated max prefixes before trusting all bytes
Preserving `str.size()` fixes short embedded-NUL bounds, but a 513+ byte
STRING/VARCHAR can still get a max smaller than its rows.
`_update_page_zonemap()` truncates to 512 bytes and
`modify_index_before_flush()` increments byte 512; Doris accepts arbitrary
bytes (including `UNHEX('FF')`), so `0xff` wraps to `0x00`. For a row `61 00 +
509*78 + ff 7a` and a `GT` bound with the same first 511 bytes followed by
`01`, the stored max ends in `00` and both segment/page pruning reject the row
even though it matches. Please use a carry-aware bytewise prefix successor (or
`pass_all` when none exists), conservatively handle already-persisted wrapped
bounds, and test >512-byte embedded-NUL/`ff` values at both map levels.
##########
regression-test/suites/datatype_p0/string/test_string_embedded_nul_zonemap.groovy:
##########
@@ -0,0 +1,79 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// A STRING / VARCHAR value may hold a 0x00 byte. The zone map bound used to be
+// parsed back with C string semantics and stopped at that byte, so a
comparison pushed into
+// the scan read a bound the data never held and dropped pages that hold
matching rows.
+//
+// Each table below holds 32 copies of one value, so the segment min and max
are that value.
+// The SUM(...) row is the per-row answer, which never goes through the zone
map; the three
+// COUNT(*) rows run the same predicates through the scan and have to agree
with it.
+suite("test_string_embedded_nul_zonemap") {
+ def load_one_value = { String table, String type, String hex ->
+ sql "DROP TABLE IF EXISTS ${table}"
+ sql """
+ CREATE TABLE ${table} (
+ id INT,
+ s ${type}
+ )
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "disable_auto_compaction" = "true"
+ )
+ """
+ def rows = (1..32).collect { "(${it}, UNHEX('${hex}'))" }.join(", ")
+ sql "INSERT INTO ${table} VALUES ${rows}"
+ }
+
+ // 'a' 0x00 'b' in a STRING column -- the 0x00 sits in the middle.
+ load_one_value("nul_inside_string", "STRING", "610062")
+ qt_inside_string_data "SELECT COUNT(*), MIN(LENGTH(s)), MAX(LENGTH(s)),
MIN(HEX(s)), MAX(HEX(s)) FROM nul_inside_string"
+ qt_inside_string_oracle "SELECT SUM(CAST(s > 'a' AS INT)), SUM(CAST(s !=
'a' AS INT)), SUM(CAST(s <= 'a' AS INT)) FROM nul_inside_string"
+ qt_inside_string_gt "SELECT COUNT(*) FROM nul_inside_string WHERE s > 'a'"
+ qt_inside_string_ne "SELECT COUNT(*) FROM nul_inside_string WHERE s != 'a'"
+ qt_inside_string_le "SELECT COUNT(*) FROM nul_inside_string WHERE s <= 'a'"
+ qt_inside_string_minmax "SELECT HEX(MIN(s)), HEX(MAX(s)) FROM
nul_inside_string"
+
+ // The same value in a VARCHAR column, which shares the zone map bound
path.
+ load_one_value("nul_inside_varchar", "VARCHAR(20)", "610062")
+ qt_inside_varchar_data "SELECT COUNT(*), MIN(LENGTH(s)), MAX(LENGTH(s)),
MIN(HEX(s)), MAX(HEX(s)) FROM nul_inside_varchar"
+ qt_inside_varchar_oracle "SELECT SUM(CAST(s > 'a' AS INT)), SUM(CAST(s !=
'a' AS INT)), SUM(CAST(s <= 'a' AS INT)) FROM nul_inside_varchar"
+ qt_inside_varchar_gt "SELECT COUNT(*) FROM nul_inside_varchar WHERE s >
'a'"
+ qt_inside_varchar_ne "SELECT COUNT(*) FROM nul_inside_varchar WHERE s !=
'a'"
+ qt_inside_varchar_le "SELECT COUNT(*) FROM nul_inside_varchar WHERE s <=
'a'"
+ qt_inside_varchar_minmax "SELECT HEX(MIN(s)), HEX(MAX(s)) FROM
nul_inside_varchar"
Review Comment:
[P1] Keep exact 512-byte maxima out of synthetic statistics
This three-byte VARCHAR case misses a wrong-result boundary in the same
path. `_update_page_zonemap()` retains an exact 512-byte max, but
`modify_index_before_flush()` increments every retained max of size 512 even
when nothing was truncated. `VARCHAR(512)` is still eligible for default
MIN/MAX pushdown, and `next_batch_of_zone_map()` emits that widened bound as
data, so a table whose sole value is `61 00 + 510*78` can return a `MAX(s)`
ending in `79` although the stored value ends in `78`. Please track whether
widening was actually needed, keep exact maxima exact, make aggregate pushdown
fall back for approximate/legacy bounds, and add an exact-512 embedded-NUL
regression here.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
