github-actions[bot] commented on code in PR #67418:
URL: https://github.com/apache/doris/pull/67418#discussion_r4228099255
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -392,13 +443,9 @@ Status
NewJsonReader::get_parsed_schema(std::vector<std::string>* col_names,
// use jsonpaths to col_names
if (!_parsed_jsonpaths.empty()) {
- for (auto& _parsed_jsonpath : _parsed_jsonpaths) {
- size_t len = _parsed_jsonpath.size();
- if (len == 0) {
- return Status::InvalidArgument("It's invalid jsonpaths.");
- }
- std::string key = _parsed_jsonpath[len - 1].key;
- col_names->emplace_back(key);
Review Comment:
[P1] Preserve JSONPath positions when the new columns are projected. With
the added fixture, `SELECT right_id` requires only that slot; FE prunes
`left_id` from the scan tuple, then BE reads `_parsed_jsonpaths[0]` for the
remaining slot and returns `left.id` (1) as `right_id` instead of `right.id`
(10). The regression selects all columns and misses this. Map each required
slot to its original JSONPath index and cover a single non-first-column
projection.
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -75,6 +76,56 @@ enum class FileCachePolicy : uint8_t;
namespace doris {
using namespace ErrorCode;
+namespace json_reader_detail {
+namespace {
+
+std::string lowercase_column_name(std::string name) {
+ std::ranges::transform(name, name.begin(),
+ [](unsigned char c) { return
static_cast<char>(std::tolower(c)); });
+ return name;
+}
+
+std::string jsonpath_column_name(const std::vector<JsonPath>& jsonpath) {
+ std::string name;
+ for (size_t i = 1; i < jsonpath.size(); ++i) {
+ if (jsonpath[i].key.empty()) {
+ continue;
+ }
+ if (!name.empty()) {
+ name += "_";
+ }
+ name += jsonpath[i].key;
+ }
+ return name.empty() ? jsonpath.back().key : name;
+}
+
+} // namespace
+
+Status derive_jsonpath_column_names(const std::vector<std::vector<JsonPath>>&
parsed_jsonpaths,
+ std::vector<std::string>* column_names) {
+ DORIS_CHECK(column_names != nullptr);
+ std::vector<std::string> leaf_names;
+ std::unordered_map<std::string, size_t> leaf_name_counts;
+ leaf_names.reserve(parsed_jsonpaths.size());
+ for (const auto& jsonpath : parsed_jsonpaths) {
+ if (jsonpath.empty()) {
+ return Status::InvalidArgument("It's invalid jsonpaths.");
+ }
+ leaf_names.emplace_back(jsonpath.back().key);
+ ++leaf_name_counts[lowercase_column_name(leaf_names.back())];
+ }
+
+ for (size_t i = 0; i < parsed_jsonpaths.size(); ++i) {
+ const auto& leaf_name = leaf_names[i];
Review Comment:
[P2] Ensure derived JSONPath names are unique across the final file schema.
For `jsonpaths=["$.left.id","$.right.id","$.left_id"]`, this emits
`left_id,right_id,left_id`; two distinct paths `$.a_b.id` and `$.a.b.id` also
both become `a_b_id`. The schema RPC sends these names to FE, whose
`fillColumns` rejects the TVF as repeated lowercase columns. Check collisions
after choosing names for every path and use an unambiguous fallback.
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -75,6 +76,56 @@ enum class FileCachePolicy : uint8_t;
namespace doris {
using namespace ErrorCode;
+namespace json_reader_detail {
+namespace {
+
+std::string lowercase_column_name(std::string name) {
+ std::ranges::transform(name, name.begin(),
+ [](unsigned char c) { return
static_cast<char>(std::tolower(c)); });
+ return name;
+}
+
+std::string jsonpath_column_name(const std::vector<JsonPath>& jsonpath) {
+ std::string name;
+ for (size_t i = 1; i < jsonpath.size(); ++i) {
+ if (jsonpath[i].key.empty()) {
+ continue;
+ }
+ if (!name.empty()) {
+ name += "_";
+ }
+ name += jsonpath[i].key;
+ }
+ return name.empty() ? jsonpath.back().key : name;
+}
+
+} // namespace
+
+Status derive_jsonpath_column_names(const std::vector<std::vector<JsonPath>>&
parsed_jsonpaths,
+ std::vector<std::string>* column_names) {
+ DORIS_CHECK(column_names != nullptr);
+ std::vector<std::string> leaf_names;
+ std::unordered_map<std::string, size_t> leaf_name_counts;
+ leaf_names.reserve(parsed_jsonpaths.size());
+ for (const auto& jsonpath : parsed_jsonpaths) {
+ if (jsonpath.empty()) {
+ return Status::InvalidArgument("It's invalid jsonpaths.");
+ }
+ leaf_names.emplace_back(jsonpath.back().key);
+ ++leaf_name_counts[lowercase_column_name(leaf_names.back())];
+ }
+
+ for (size_t i = 0; i < parsed_jsonpaths.size(); ++i) {
+ const auto& leaf_name = leaf_names[i];
+
column_names->emplace_back(leaf_name_counts[lowercase_column_name(leaf_name)] >
1
Review Comment:
[P2] Avoid generated names colliding with path-partition columns. With
`jsonpaths=["$.left.id","$.right.id"]` and `path_partition_keys=left_id`, this
creates a JSON file column `left_id`, and FE then appends a partition column
with the same name. `FileQueryScanNode` classifies both as partition keys, so
the JSON `left.id` field loses its separate identity and is not read as a file
slot. Validate the combined TVF schema or choose a collision-free file name.
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -75,6 +76,56 @@ enum class FileCachePolicy : uint8_t;
namespace doris {
using namespace ErrorCode;
+namespace json_reader_detail {
+namespace {
+
+std::string lowercase_column_name(std::string name) {
+ std::ranges::transform(name, name.begin(),
Review Comment:
[P2] Match FE's case folding for Unicode JSONPath leaves. For
`["$.left.É","$.right.é"]`, bytewise `std::tolower` leaves the UTF-8 names
distinct, so the new helper returns `É` and `é`; FE lowercases both to `é` and
rejects the TVF, even though the paths select different keys. This input was
rejected before the PR, and the new disambiguation still does not cover it. Use
compatible normalization or a final FE-aware uniqueness pass.
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -75,6 +76,56 @@ enum class FileCachePolicy : uint8_t;
namespace doris {
using namespace ErrorCode;
+namespace json_reader_detail {
+namespace {
+
+std::string lowercase_column_name(std::string name) {
+ std::ranges::transform(name, name.begin(),
+ [](unsigned char c) { return
static_cast<char>(std::tolower(c)); });
+ return name;
+}
+
+std::string jsonpath_column_name(const std::vector<JsonPath>& jsonpath) {
+ std::string name;
+ for (size_t i = 1; i < jsonpath.size(); ++i) {
+ if (jsonpath[i].key.empty()) {
+ continue;
+ }
+ if (!name.empty()) {
+ name += "_";
+ }
Review Comment:
[P2] Include array indices when naming distinct JSONPaths. Paths
`$.items[0].id` and `$.items[1].id` select different array elements, but this
code joins only their keys and names both columns `items_id`. FE rejects the
resulting schema, so the two values cannot be queried together. Preserve
supported numeric indices in generated names and test this case.
##########
regression-test/suites/external_table_p0/tvf/test_jsonpaths_duplicate_leaf.groovy:
##########
@@ -0,0 +1,63 @@
+// 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.
+
+suite("test_jsonpaths_duplicate_leaf", "p0,external") {
+ String ak = getS3AK()
+ String sk = getS3SK()
+ String s3Endpoint = getS3Endpoint()
+ String bucket = context.config.otherConfigs.get("s3BucketName")
+ String uri =
"https://${bucket}.${s3Endpoint}/regression/tvf/test_jsonpaths_duplicate_leaf/nested_duplicate.json";
Review Comment:
[P2] Stage the JSON fixture before querying its S3 URI. This suite commits
`nested_duplicate.json` under `regression-test/data` but never uploads it to
the new `regression/tvf` key. With a fresh bucket, `parseFile` finds no object
and FE exposes only `__dummy_col`, so these queries fail to bind before testing
the JSONPath change. Upload the fixture in suite setup or read the committed
file through a local/HDFS TVF.
--
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]
