github-actions[bot] commented on code in PR #63110:
URL: https://github.com/apache/doris/pull/63110#discussion_r3260547268
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -899,6 +907,14 @@ private void computeTabletInfo() throws UserException {
}
}
+ if (specifiedTabletIds != null && !specifiedTabletIds.isEmpty()) {
+ if (prunedTabletIds != null) {
Review Comment:
This applies the same `specifiedTabletIds` set to every selected partition.
For a partitioned table, `binlog("table"="t", "tablet"="<tablet in p1>")`
without an explicit partition first selects all partitions, then partitions
that do not own that tablet reach the `selectedTable.getTablet(id) == null`
branch and throw `IllegalStateException("tablet ... does not exist")`. The
tablet option should either resolve/filter to the owning partition(s), or skip
partitions with no intersection instead of treating the tablet as missing
globally.
##########
regression-test/suites/tso_p0/test_tso_rowset_commit_tso.groovy:
##########
@@ -18,61 +18,51 @@
import org.apache.doris.regression.util.Http
suite("test_tso_rowset_commit_tso", "nonConcurrent") {
- def tsoFeatureConfig = sql "SHOW FRONTEND CONFIG like
'%experimental_enable_tso_feature%';"
- def tsoPersistConfig = sql "SHOW FRONTEND CONFIG like
'%enable_tso_persist_journal%';"
- logger.info("${tsoFeatureConfig}")
- logger.info("${tsoPersistConfig}")
- try {
- sql "ADMIN SET FRONTEND CONFIG ('enable_tso_persist_journal' = 'true')"
- sql "ADMIN SET FRONTEND CONFIG ('experimental_enable_tso_feature' =
'true')"
- sleep(1000)
- def masterFeHttpAddress = "${getMasterIp()}:${getMasterPort('http')}"
- def url = String.format("http://%s/api/tso";, masterFeHttpAddress)
- def tsoResp = Http.GET(url, true)
- if (tsoResp.code != 0) {
- logger.info("tso api not available, skip
test_tso_rowset_commit_tso")
- return
- }
+ def masterFeHttpAddress = "${getMasterIp()}:${getMasterPort('http')}"
+ def url = String.format("http://%s/api/tso";, masterFeHttpAddress)
+ def tsoResp = Http.GET(url, true, true)
+ if (tsoResp.code != 0) {
+ logger.info("tso api not available, skip test_tso_rowset_commit_tso")
+ return
+ }
- def tableName = "test_tso_rowset_commit_tso"
- sql """DROP TABLE IF EXISTS ${tableName}"""
- sql """
+ def tableName = "test_tso_rowset_commit_tso"
+ sql """DROP TABLE IF EXISTS ${tableName}"""
+ sql """
CREATE TABLE IF NOT EXISTS ${tableName} (
id INT
)
DISTRIBUTED BY HASH(id) BUCKETS 1
PROPERTIES ("replication_num" = "1", "enable_tso" = "true",
"disable_auto_compaction" = "true")
"""
- sql """INSERT INTO ${tableName} VALUES (1), (2), (3)"""
+ sql """INSERT INTO ${tableName} VALUES (1), (2), (3)"""
- def tablets = sql_return_maparray """ show tablets from ${tableName};
"""
- assertTrue(tablets.size() > 0)
- def tabletId = tablets[0]["TabletId"]
+ def tablets = sql_return_maparray """ show tablets from ${tableName}; """
+ assertTrue(tablets.size() > 0)
+ def tabletId = tablets[0]["TabletId"]
- def commitTso = -1L
- for (int i = 0; i < 10; i++) {
- def rowsets = sql_return_maparray """
+ def commitTso = -1L
+ for (int i = 0; i < 10; i++) {
+ def rowsets = sql_return_maparray """
select COMMIT_TSO from information_schema.rowsets
where TABLET_ID = ${tabletId}
order by TXN_ID desc limit 1
"""
- if (rowsets.size() > 0) {
- commitTso = ((Number) rowsets[0]["COMMIT_TSO"]).longValue()
- }
- if (commitTso > 0) {
- break
- }
- Thread.sleep(1000)
+ if (rowsets.size() > 0) {
+ def matcher = rowsets[0]["COMMIT_TSO"].toString() =~
/\[(-?\d+)-(-?\d+)\]/
+ assertTrue(matcher.matches())
+ assertEquals(matcher[0][1], matcher[0][2])
+ commitTso = Long.parseLong(matcher[0][2])
+ }
+ if (commitTso > 0) {
+ break
}
+ Thread.sleep(1000)
+ }
- assertTrue(commitTso > 0)
- assertTrue(commitTso >= ((Number)
tsoResp.data.current_tso).longValue())
+ assertTrue(commitTso > 0)
+ assertTrue(commitTso >= ((Number) tsoResp.data.current_tso).longValue())
- sql """DROP TABLE IF EXISTS ${tableName}"""
- } finally {
- sql "ADMIN SET FRONTEND CONFIG ('experimental_enable_tso_feature' =
'false')"
- sql "ADMIN SET FRONTEND CONFIG ('enable_tso_persist_journal' =
'${tsoPersistConfig[0][1]}')"
- sql "ADMIN SET FRONTEND CONFIG ('experimental_enable_tso_feature' =
'${tsoFeatureConfig[0][1]}')"
- }
+ sql """DROP TABLE IF EXISTS ${tableName}"""
}
Review Comment:
Regression tests should drop tables before use, not after, so a failed run
preserves the table for debugging. This table is already dropped before
creation, so please remove the final drop.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/TableBinlogFunction.java:
##########
@@ -0,0 +1,211 @@
+// 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.
+
+package org.apache.doris.tablefunction;
+
+import org.apache.doris.analysis.TupleDescriptor;
+import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.DatabaseIf;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.OlapTable;
+import org.apache.doris.catalog.Partition;
+import org.apache.doris.catalog.RowBinlogTableWrapper;
+import org.apache.doris.catalog.TableIf;
+import org.apache.doris.catalog.TableIf.TableType;
+import org.apache.doris.catalog.info.PartitionNamesInfo;
+import org.apache.doris.common.AnalysisException;
+import org.apache.doris.common.Config;
+import org.apache.doris.common.MetaNotFoundException;
+import org.apache.doris.planner.OlapScanNode;
+import org.apache.doris.planner.PlanNodeId;
+import org.apache.doris.planner.ScanContext;
+import org.apache.doris.planner.ScanNode;
+import org.apache.doris.qe.ConnectContext;
+import org.apache.doris.qe.SessionVariable;
+
+import com.google.common.base.Strings;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+/**
+ * The implementation of table valued function `binlog`.
+ *
+ * This TVF is debug-only, used to read binlog<row> data from a table.
+ */
+public class TableBinlogFunction extends TableValuedFunctionIf {
+ public static final String NAME = "binlog";
Review Comment:
`binlog()` exposes the hidden row-binlog contents of an OLAP table, but this
class does not override `checkAuth`, so it inherits the no-op default from
`TableValuedFunctionIf`. `CheckPrivileges` only calls `tvf.checkAuth(ctx)`, so
any user who can submit this TVF can read row-binlog data for `db.table`
without `SELECT` or admin privilege. Please add an authorization check against
the target table before planning the scan.
##########
regression-test/suites/tso_p0/test_tso_rowset_commit_tso.groovy:
##########
@@ -18,61 +18,51 @@
import org.apache.doris.regression.util.Http
suite("test_tso_rowset_commit_tso", "nonConcurrent") {
- def tsoFeatureConfig = sql "SHOW FRONTEND CONFIG like
'%experimental_enable_tso_feature%';"
- def tsoPersistConfig = sql "SHOW FRONTEND CONFIG like
'%enable_tso_persist_journal%';"
- logger.info("${tsoFeatureConfig}")
- logger.info("${tsoPersistConfig}")
- try {
- sql "ADMIN SET FRONTEND CONFIG ('enable_tso_persist_journal' = 'true')"
- sql "ADMIN SET FRONTEND CONFIG ('experimental_enable_tso_feature' =
'true')"
- sleep(1000)
- def masterFeHttpAddress = "${getMasterIp()}:${getMasterPort('http')}"
- def url = String.format("http://%s/api/tso";, masterFeHttpAddress)
- def tsoResp = Http.GET(url, true)
- if (tsoResp.code != 0) {
- logger.info("tso api not available, skip
test_tso_rowset_commit_tso")
- return
- }
+ def masterFeHttpAddress = "${getMasterIp()}:${getMasterPort('http')}"
+ def url = String.format("http://%s/api/tso";, masterFeHttpAddress)
+ def tsoResp = Http.GET(url, true, true)
+ if (tsoResp.code != 0) {
+ logger.info("tso api not available, skip test_tso_rowset_commit_tso")
+ return
+ }
- def tableName = "test_tso_rowset_commit_tso"
- sql """DROP TABLE IF EXISTS ${tableName}"""
- sql """
+ def tableName = "test_tso_rowset_commit_tso"
+ sql """DROP TABLE IF EXISTS ${tableName}"""
Review Comment:
Doris regression standards require ordinary single-table tests to hardcode
the table name instead of using `def tableName` and SQL interpolation. Please
use the literal `test_tso_rowset_commit_tso` in the SQL statements.
##########
gensrc/proto/olap_file.proto:
##########
@@ -173,7 +178,11 @@ message RowsetMetaPB {
optional bool is_recycled = 1013; // for recycler mark rowset as recycled
optional string job_id = 1014;
- optional int64 commit_tso = 1015 [default = -1];
+ optional TsoRangePB commit_tso = 1015;
+
Review Comment:
Changing persisted field `commit_tso` from `int64` to message `TsoRangePB`
while reusing tag `1015` is not protobuf-compatible. Existing rowset meta
encodes this tag as a varint, which new code will ignore as an unknown field;
new code then reads `start_tso/end_tso` defaults as `-1`, and row-binlog paths
convert that to `0`. Mixed-version nodes also cannot read the new
length-delimited value. Please keep the old field for compatibility and add the
range under a new tag, with migration/read fallback from the old scalar value.
The same concern applies to `RowsetMetaCloudPB.commit_tso` below.
--
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]
