github-actions[bot] commented on code in PR #68853:
URL: https://github.com/apache/doris/pull/68853#discussion_r4242705286
##########
be/src/format/json/new_json_reader.cpp:
##########
@@ -1147,7 +1147,26 @@ Status
NewJsonReader::_simdjson_write_data_to_column(simdjson::ondemand::value&
}
auto primitive_type = type_desc->get_primitive_type();
- if (_is_load || !is_complex_type(primitive_type)) {
+ if (primitive_type == TYPE_FILE) {
+ std::string_view json_str = simdjson::to_json_string(value);
+ Slice slice {json_str.data(), json_str.size()};
+ const auto status =
+ data_serde->deserialize_one_cell_from_json(*data_column_ptr,
slice, _serde_options);
+ if (!status.ok()) {
+ if (!_is_load) return status;
+ if (_params.strict_mode || nullable_column == nullptr) {
Review Comment:
[P1] Roll back the whole row when strict FILE decoding fails. If an earlier
JSON field such as `id` has already been appended, this branch reports the bad
FILE and returns OK with `valid=false` but appends no FILE value.
`_simdjson_set_column_value` then exits without truncating the earlier column;
the simple-JSON caller counts the row, so later valid rows have different
column offsets and FileScanner can hit its size assertion or process shifted
values. Reject the row atomically and test a bad top-level FILE after `id`,
followed by a valid row, with `strict_mode=true`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/ExpressionEstimation.java:
##########
@@ -155,8 +160,31 @@ public ColumnStatistic visit(Expression expr, Statistics
context) {
return expr.child(0).accept(this, context);
}
+ @Override
+ public ColumnStatistic visitIsNull(IsNull isNull, Statistics context) {
+ if (!isNull.child().getDataType().isFileType()) {
Review Comment:
[P2] Complement the Boolean range for FILE `IS NOT NULL`. SQL represents it
as `Not(IsNull(f))`, but the generic visitor copies the exact `IS NULL` range
computed here. For all-null FILE input, `Project(p := f IS NOT NULL)` therefore
records `p` as always true although every value is false. In `Filter(p IN
(FALSE, NULL)) -> Project(p := NOT ISNULL(f)) -> Limit -> UnionAll(typed-null
FILE arms)`, the IN estimator can exclude both options from that wrong [1,1]
range and estimate zero rows even though every row matches FALSE. Estimate the
Not form and cover all-null and no-null FILE projections.
##########
be/src/exec/common/data_gen_functions/vlist_file_tvf.cpp:
##########
@@ -0,0 +1,148 @@
+// 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.
+
+#include "exec/common/data_gen_functions/vlist_file_tvf.h"
+
+#include <gen_cpp/PaloInternalService_types.h>
+#include <gen_cpp/PlanNodes_types.h>
+
+#include <algorithm>
+#include <array>
+
+#include "core/data_type/data_type_file.h"
+#include "core/field.h"
+#include "core/value/file_value.h"
+#include "io/file_factory.h"
+#include "io/fs/s3_file_system.h"
+#include "runtime/runtime_state.h"
+#include "util/s3_uri.h"
+
+namespace doris {
+VListFileTVF::VListFileTVF(TupleId tuple_id, const TupleDescriptor* tuple_desc)
+ : VDataGenFunctionInf(tuple_id, tuple_desc) {}
+
+VListFileTVF::~VListFileTVF() = default;
+
+Status VListFileTVF::set_scan_ranges(const std::vector<TScanRangeParams>&
scan_ranges) {
+ DCHECK(scan_ranges.size() == 1);
+ const auto& params =
scan_ranges[0].scan_range.data_gen_scan_range.list_file_params;
+ const auto key_start = params.uri.find('/', 5);
+ S3URI uri(params.uri.substr(0, key_start));
+ RETURN_IF_ERROR(uri.parse());
+ _properties = params.resource.properties;
+ _path.bucket = uri.get_bucket();
+ _path.prefix = key_start == std::string::npos ? "" :
params.uri.substr(key_start + 1);
+ if (!_path.prefix.empty() && !_path.prefix.ends_with('/')) {
+ _path.prefix += '/';
+ }
+ _path.delimiter = params.recursive ? "" : "/";
Review Comment:
[P2] Preserve nonrecursive listing for Azure resources. This sets `/` as the
only depth control when `recursive=false`, but
`AzureObjStorageClient::list_objects_page` ignores `ObjStoragePath.delimiter`
and uses flat `ListBlobs`. The output loop filters only keys ending in `/`, so
a request for `dir/` includes `dir/deep/a.txt` alongside direct children on
Azure-enabled builds. Apply delimiter semantics in the Azure client or filter
descendants before emitting them, and cover an Azure resource with nested keys.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/ExpressionEstimation.java:
##########
@@ -277,6 +360,10 @@ private ColumnStatistic castMinMax(ColumnStatistic
colStats, DataType targetType
@Override
public ColumnStatistic visitLiteral(Literal literal, Statistics context) {
+ if (literal.getDataType().isFileType() && literal.isNullLiteral()) {
Review Comment:
[P2] Estimate non-null FILE literals from their payload. This special case
covers only typed NULL; a folded non-null `FileLiteral` reaches the numeric
literal path below, where `getDouble()` becomes 0 and `avgSizeByte` is fixed at
the 16-byte slot width. A constant FILE with large `inline` bytes is therefore
severely undercosted in projections and one-row plans, and carries meaningless
numeric min/max and hot values. Use its six fields to derive byte size and
leave numeric extrema unavailable, as the other FILE statistics paths do.
##########
be/src/exec/common/data_gen_functions/vlist_file_tvf.cpp:
##########
@@ -0,0 +1,148 @@
+// 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.
+
+#include "exec/common/data_gen_functions/vlist_file_tvf.h"
+
+#include <gen_cpp/PaloInternalService_types.h>
+#include <gen_cpp/PlanNodes_types.h>
+
+#include <algorithm>
+#include <array>
+
+#include "core/data_type/data_type_file.h"
+#include "core/field.h"
+#include "core/value/file_value.h"
+#include "io/file_factory.h"
+#include "io/fs/s3_file_system.h"
+#include "runtime/runtime_state.h"
+#include "util/s3_uri.h"
+
+namespace doris {
+VListFileTVF::VListFileTVF(TupleId tuple_id, const TupleDescriptor* tuple_desc)
+ : VDataGenFunctionInf(tuple_id, tuple_desc) {}
+
+VListFileTVF::~VListFileTVF() = default;
+
+Status VListFileTVF::set_scan_ranges(const std::vector<TScanRangeParams>&
scan_ranges) {
+ DCHECK(scan_ranges.size() == 1);
+ const auto& params =
scan_ranges[0].scan_range.data_gen_scan_range.list_file_params;
+ const auto key_start = params.uri.find('/', 5);
+ S3URI uri(params.uri.substr(0, key_start));
+ RETURN_IF_ERROR(uri.parse());
+ _properties = params.resource.properties;
+ _path.bucket = uri.get_bucket();
+ _path.prefix = key_start == std::string::npos ? "" :
params.uri.substr(key_start + 1);
+ if (!_path.prefix.empty() && !_path.prefix.ends_with('/')) {
+ _path.prefix += '/';
+ }
+ _path.delimiter = params.recursive ? "" : "/";
+ static constexpr std::array<std::string_view, 2> NAMES {"path", "file"};
+ _column_indices.clear();
+ for (const auto* slot : _tuple_desc->slots()) {
+ const auto* const field = std::ranges::find(NAMES, slot->col_name());
+ DCHECK(field != NAMES.end());
+ _column_indices.push_back(std::distance(NAMES.begin(), field));
+ }
+ return Status::OK();
+}
+
+Status VListFileTVF::_fetch_next_page(RuntimeState* state) {
+ RETURN_IF_CANCELLED(state);
+ if (!_filesystem) {
+ io::FSPropertiesRef properties(TFileType::FILE_S3);
+ properties.properties = &_properties;
+ io::FileDescription description;
+ description.path = "s3://" + _path.bucket;
+ auto filesystem = DORIS_TRY(FileFactory::create_fs(properties,
description));
+ _filesystem = std::static_pointer_cast<io::S3FileSystem>(filesystem);
+ RETURN_IF_CANCELLED(state);
+ }
+
+ RETURN_IF_CANCELLED(state);
+ const auto client = _filesystem->client_holder()->get();
+ // Release the previous page before the provider allocates its replacement.
+ _objects = std::vector<ObjectMeta>();
+ auto page = client->list_objects_page(_path, _continuation_token);
+ RETURN_IF_CANCELLED(state);
+ if (!page.resp.ok()) {
+ return {page.resp.status.code, std::move(page.resp.status.msg)};
+ }
+ _objects = std::move(page.objects);
+ _next_index = 0;
+ _continuation_token = std::move(page.continuation_token);
+ _has_more = page.has_more;
+ return Status::OK();
+}
+
+Status VListFileTVF::get_next(RuntimeState* state, Block* block, bool* eos) {
+ DCHECK(block->rows() == 0);
+ DCHECK(!_tuple_desc->slots().empty());
+ RETURN_IF_CANCELLED(state);
+ *eos = false;
+
+ // Retain the mutable owner locally. Only publish after remote I/O and
FILE validation succeed,
+ // so an error cannot leave a reused block with a moved-from column.
+ MutableColumns columns;
+ for (const auto* slot : _tuple_desc->slots()) {
+ columns.push_back(slot->get_empty_mutable_column());
+ }
+ auto& output = columns[0];
+ while (output->size() < static_cast<size_t>(state->batch_size())) {
+ if (_next_index == _objects.size()) {
+ if (!output->empty()) {
+ break;
+ }
+ if (!_has_more) {
+ *eos = true;
+ break;
+ }
+ RETURN_IF_ERROR(_fetch_next_page(state));
+ continue;
+ }
+ RETURN_IF_CANCELLED(state);
+ const auto& object = _objects[_next_index++];
+ if (object.key.ends_with('/')) {
+ continue;
+ }
+ File value(DataTypeFile::FIELD_COUNT);
+ value[0] = Field::create_field<TYPE_STRING>("s3://" + _path.bucket +
"/" + object.key);
Review Comment:
[P2] Make LIST_FILE paths round-trip through TO_FILE for raw S3 keys. This
concatenation emits keys such as `http://dir/a.txt` or `file?#.txt` unchanged,
but `TO_FILE` passes the resulting URI to `S3URI::parse()`, which rejects the
extra `://` or truncates the key at `?`/`#` before HEAD. The added LIST_FILE
tests allow these keys, so a listed path can fail or resolve a different
object. Use one lossless representation and cover those keys in the
LIST_FILE-to-TO_FILE test.
--
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]
