github-actions[bot] commented on code in PR #61373:
URL: https://github.com/apache/doris/pull/61373#discussion_r2938576175
##########
sdk/go-doris-sdk/pkg/load/loader/request_builder.go:
##########
@@ -65,6 +82,17 @@ func CreateStreamLoadRequest(cfg *config.Config, data
io.Reader, attempt int) (*
// Construct the load URL
loadURL := fmt.Sprintf(StreamLoadPattern, host, cfg.Database, cfg.Table)
+ // Wrap with gzip compression if enabled.
+ // Must happen before http.NewRequest so the compressed *bytes.Reader
is used as body,
+ // allowing Go to auto-set GetBody for transparent 307 redirect replay.
+ if cfg.EnableGzip {
+ compressed, err := wrapWithGzip(data)
Review Comment:
**Issue (Medium): Gzip compression is re-done on every retry attempt.**
Looking at the retry flow in `doris_load_client.go`:
1. `getBodyFunc()` (line 254) returns a fresh **uncompressed** reader for
each attempt
2. `CreateStreamLoadRequest()` is called with this uncompressed reader
3. `wrapWithGzip()` re-compresses the entire payload each time
For large payloads, this is wasteful. Consider either:
- Moving gzip compression to happen once in `doris_load_client.go` before
the retry loop (wrap the `getBodyFunc` to return already-compressed data), or
- Caching the compressed bytes after the first compression and returning a
`bytes.NewReader` over the cached compressed bytes on subsequent attempts.
##########
sdk/go-doris-sdk/cmd/examples/main.go:
##########
@@ -75,6 +75,9 @@ func runExample(name string) {
case "basic":
fmt.Println("Running Basic Concurrent Example...")
examples.RunBasicConcurrentExample()
+ case "gzip":
+ fmt.Println("Running Gzip Compression Example...")
Review Comment:
**Issue (Low): The `"all"` case (line 81-92) does not include
`GzipExample()`, and the `usage` const (line 31-58) does not list the `gzip`
option.**
Running `go run cmd/examples/main.go all` will skip the gzip example, and
the help text won't mention it. Both should be updated for consistency.
##########
sdk/go-doris-sdk/examples/gzip_example.go:
##########
@@ -0,0 +1,65 @@
+// 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 examples
+
+import (
+ "fmt"
+ "strings"
+
+ doris "github.com/apache/doris/sdk/go-doris-sdk"
+)
+
+func GzipExample() {
+ config := &doris.Config{
+ Endpoints: []string{"http://10.16.10.6:48939"},
+ User: "root",
+ Password: "",
Review Comment:
**Issue (Low): Hardcoded internal IP address.**
The endpoint `http://10.16.10.6:48939` appears to be an internal/development
address. Other examples in this SDK use similar patterns, but for a public
example this should use a more generic placeholder like `http://127.0.0.1:8030`
(which is what the README example uses).
##########
sdk/go-doris-sdk/pkg/load/loader/request_builder.go:
##########
@@ -149,6 +177,11 @@ func buildStreamLoadOptions(cfg *config.Config)
map[string]string {
// Don't add group_commit option
}
+ // Add compress_type header if gzip is enabled
+ if cfg.EnableGzip {
+ result["compress_type"] = "gz"
Review Comment:
**Issue (Low): No conflict detection with user-set `compress_type` in
`Options`.**
If a user sets both `EnableGzip: true` and `Options:
map[string]string{"compress_type": "lz4"}`, the `EnableGzip` silently wins
because it's applied after user options (line 181-183 overwrites whatever was
in `result["compress_type"]`). Consider either:
- Detecting and returning an error in `ValidateInternal()` if `Options`
contains `compress_type` while `EnableGzip` is true, or
- Documenting that `EnableGzip` takes precedence.
##########
sdk/go-doris-sdk/pkg/load/config/load_config.go:
##########
@@ -141,6 +142,12 @@ func (c *Config) ValidateInternal() error {
return fmt.Errorf("format cannot be nil")
}
+ if c.EnableGzip {
+ if _, ok := c.Format.(*CSVFormat); !ok {
Review Comment:
**Issue (High): The assumption that Doris `compress_type` does not support
JSON is incorrect.**
I traced through the Doris BE code:
1. `be/src/util/load_util.cpp` `LoadUtil::parse_format()` parses
`compress_type` independently of the format — it sets `TFileCompressType` for
both CSV and JSON.
2. `be/src/format/json/new_json_reader.cpp` reads `_params.compress_type`,
creates a `Decompressor`, and passes it to `NewPlainTextLineReader` for
transparent decompression.
3. The only difference is that CSV gets compress-specific `TFileFormatType`
values (e.g., `FORMAT_CSV_GZ`), while JSON always uses `FORMAT_JSON` and relies
solely on the separate `compress_type` field.
This validation incorrectly rejects a valid and functional combination.
`EnableGzip` should work with JSON format as well. Please remove this
restriction, or if you want to be conservative, at least change the error
message to not make a false claim, and file a follow-up to enable JSON support
after testing.
--
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]
