Copilot commented on code in PR #55651:
URL: https://github.com/apache/doris/pull/55651#discussion_r2348082103
##########
regression-test/suites/external_table_p0/jdbc/type_test/tvf/test_mysql_all_types_tvf.groovy:
##########
@@ -23,7 +23,6 @@ suite("test_mysql_all_types_tvf",
"p0,external,mysql,external_docker,external_do
String driver_url =
"https://${bucket}.${s3_endpoint}/regression/jdbc_driver/mysql-connector-j-8.3.0.jar";
if (enabled != null && enabled.equalsIgnoreCase("true")) {
String mysql_port = context.config.otherConfigs.get("mysql_57_port");
Review Comment:
[nitpick] Remove the trailing empty line at line 25 to maintain consistent
spacing with the existing codebase pattern.
##########
regression-test/suites/external_table_p0/jdbc/type_test/ctas/test_mysql_all_types_ctas.groovy:
##########
@@ -40,11 +40,11 @@ suite("test_mysql_all_types_ctas",
"p0,external,mysql,external_docker,external_d
sql """use internal.test_mysql_all_types_ctas;"""
- sql """create table all_types_nullable properties("replication_num" =
"1") as select * from mysql_all_type_ctas_test.doris_test.all_types_nullable;"""
+ sql """create table all_types_nullable properties("replication_num" =
"1") as select * EXCEPT(`binary`,`varbinary`) from
mysql_all_type_ctas_test.doris_test.all_types_nullable;"""
Review Comment:
[nitpick] There's inconsistent spacing: line 47 has an extra space before
the asterisk ('select * EXCEPT') while line 43 correctly uses 'select *
EXCEPT'. Remove the extra space for consistency.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalOneRowRelation.java:
##########
@@ -174,6 +174,19 @@ public Optional<ResultSet> computeResultInFe(
if (expr instanceof Literal) {
LiteralExpr legacyExpr = ((Literal)
expr).toLegacyLiteral();
columns.add(new Column(output.getName(),
output.getDataType().toCatalogDataType()));
+ if
(output.getDataType().toCatalogDataType().isVarbinaryType()) {
+ // The FE (computeResultInFe) can currently only build
a ResultSet<List<List<String>>>.
+ // If we materialize a VARBINARY literal via
legacyExpr.getStringValueForQuery():
+ // 1) We first wrap the raw bytes in a Java String.
+ // 2) Later StmtExecutor.sendTextResultRow
re-encodes that String as UTF-8 when
+ // writing the MySQL wire protocol. This may
expand bytes (e.g. 0xAB becomes
+ // two bytes 0xC2 0xAB), so the client observes a
different value than the BE path.
+ // The BE execution path is correct because it sends
the raw bytes already serialized
+ // for the MySQL protocol without a lossy String
round-trip.
+ // Todo: Refactor ResultSet (supports per-cell byte[])
+ // so we can VARBINARY safely and remove this early
return.
Review Comment:
[nitpick] The comment explaining why VARBINARY returns Optional.empty() is
helpful, but it should clarify the specific impact on users. Consider adding
that this limitation means VARBINARY literals in SELECT statements will be
executed in the BE rather than computed in the FE, which may have performance
implications.
```suggestion
// so we can VARBINARY safely and remove this early
return.
//
// User impact: This limitation means that VARBINARY
literals in SELECT statements
// will be executed in the BE (Backend) rather than
computed in the FE (Frontend),
// which may have performance implications.
```
##########
regression-test/suites/external_table_p0/jdbc/type_test/ctas/test_mysql_all_types_ctas.groovy:
##########
@@ -40,11 +40,11 @@ suite("test_mysql_all_types_ctas",
"p0,external,mysql,external_docker,external_d
sql """use internal.test_mysql_all_types_ctas;"""
- sql """create table all_types_nullable properties("replication_num" =
"1") as select * from mysql_all_type_ctas_test.doris_test.all_types_nullable;"""
+ sql """create table all_types_nullable properties("replication_num" =
"1") as select * EXCEPT(`binary`,`varbinary`) from
mysql_all_type_ctas_test.doris_test.all_types_nullable;"""
qt_select_all_types_nullable """select * from
internal.test_mysql_all_types_ctas.all_types_nullable order by 1;"""
- sql """create table all_types_non_nullable
properties("replication_num" = "1") as select * from
mysql_all_type_ctas_test.doris_test.all_types_non_nullable;"""
+ sql """create table all_types_non_nullable
properties("replication_num" = "1") as select * EXCEPT(`binary`,`varbinary`)
from mysql_all_type_ctas_test.doris_test.all_types_non_nullable;"""
Review Comment:
[nitpick] There's inconsistent spacing: line 47 has an extra space before
the asterisk ('select * EXCEPT') while line 43 correctly uses 'select *
EXCEPT'. Remove the extra space for consistency.
```suggestion
sql """create table all_types_non_nullable
properties("replication_num" = "1") as select * EXCEPT(`binary`,`varbinary`)
from mysql_all_type_ctas_test.doris_test.all_types_non_nullable;"""
```
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/util/JdbcUtils.groovy:
##########
@@ -24,9 +24,12 @@ import java.sql.Connection
import java.sql.PreparedStatement
import java.sql.ResultSet
import java.sql.ResultSetMetaData
-import java.sql.Types;
+import java.sql.Types
Review Comment:
[nitpick] The import statement on line 27 is missing a semicolon, while the
other modified imports have semicolons. This creates inconsistent formatting in
the import block.
```suggestion
import java.sql.Types;
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalOneRowRelation.java:
##########
@@ -174,6 +174,19 @@ public Optional<ResultSet> computeResultInFe(
if (expr instanceof Literal) {
LiteralExpr legacyExpr = ((Literal)
expr).toLegacyLiteral();
columns.add(new Column(output.getName(),
output.getDataType().toCatalogDataType()));
+ if
(output.getDataType().toCatalogDataType().isVarbinaryType()) {
Review Comment:
[nitpick] The comment explaining why VARBINARY returns Optional.empty() is
helpful, but it should clarify the specific impact on users. Consider adding
that this limitation means VARBINARY literals in SELECT statements will be
executed in the BE rather than computed in the FE, which may have performance
implications.
--
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]
