morningman commented on code in PR #70232:
URL: https://github.com/apache/doris/pull/70232#discussion_r4475602090
##########
regression-test/suites/external_table_p0/tvf/test_http_tvf.groovy:
##########
@@ -349,71 +354,71 @@ suite("test_http_tvf", "p0") {
);
"""
- // hf
- def hfCountOneFile = sql """
+ // Stable mirrors of the HuggingFace fixtures. Keep the regression
independent
+ // from public HuggingFace availability and mutable dataset branches.
+ qt_sql15 """
select count(*) from
http(
- "uri" =
"hf://datasets/fka/awesome-chatgpt-prompts/blob/main/prompts.csv",
- "format" = "csv"
+ "uri" =
"${stableRemoteDataUrl}/awesome-chatgpt-prompts/61229c8c07ea12c6e219b2b2b728f23f2842901b/prompts.csv",
Review Comment:
The facts are right, and this is the trade-off the description lists under
"Notes for Reviewers": P0 no longer drives `hf://` end to end, and sql16, sql20
and the second sql21 now repeat sql15 / sql19. The duplicates stay so that the
suite and its goldens match branch-4.1 and branch-4.2, where #65877, #66013 and
#66027 come from; the sql15 to sql21 blocks of the `.out` are identical on all
three branches.
Two points on scope:
- Not all `hf://` coverage is gone. FE UT `HFUtilsTest` covers `@revision`
parsing, URL conversion and glob matching (`*`, `**`, `[...]`), and
`testGlobExpansionExceptions`
(`fe/fe-core/src/test/java/org/apache/doris/tablefunction/HFUtilsTest.java:406-413`)
expands `hf://datasets/fka/awesome-chatgpt-prompts/blob/main/*` against the
live HuggingFace tree API on every FE UT run (the last 300 runs passed). What
this PR drops is the multi-level listing (`*/...`, `**`), `@revision` against a
real non-main branch, and BE reads of `resolve/` URLs.
- A hermetic equivalent can't be built from the suite. The endpoint is the
constant `DEFAULT_ENDPOINT = "https://huggingface.co"`
(`fe/fe-core/src/main/java/org/apache/doris/tablefunction/HFUtils.java:66`),
and nothing in the URI or the TVF properties changes it, so pointing `hf://` at
the in-runner server needs an FE change (a configurable endpoint). That is a
separate change, not part of this test-only port. Deterministic `hf://` cases
against huggingface.co would keep the dependency this PR removes: on branch-4.1
the suite failed with `Connect timed out` to HuggingFace (#66013), and on
master the HuggingFace part takes about 146 of the suite's roughly 150 seconds.
So I'm keeping this part as ported.
##########
regression-test/suites/external_table_p0/tvf/test_http_tvf.groovy:
##########
@@ -349,71 +354,71 @@ suite("test_http_tvf", "p0") {
);
"""
- // hf
- def hfCountOneFile = sql """
+ // Stable mirrors of the HuggingFace fixtures. Keep the regression
independent
+ // from public HuggingFace availability and mutable dataset branches.
+ qt_sql15 """
select count(*) from
http(
- "uri" =
"hf://datasets/fka/awesome-chatgpt-prompts/blob/main/prompts.csv",
- "format" = "csv"
+ "uri" =
"${stableRemoteDataUrl}/awesome-chatgpt-prompts/61229c8c07ea12c6e219b2b2b728f23f2842901b/prompts.csv",
+ "format" = "csv",
+ "skip_lines" = "1"
);
"""
- assertTrue(Long.parseLong(hfCountOneFile[0][0].toString()) > 0)
- def hfCountWildcard = sql """
+ qt_sql16 """
select count(*) from
http(
- "uri" =
"hf://datasets/fka/awesome-chatgpt-prompts/blob/main/*.csv",
- "format" = "csv"
+ "uri" =
"${stableRemoteDataUrl}/awesome-chatgpt-prompts/61229c8c07ea12c6e219b2b2b728f23f2842901b/prompts.csv",
+ "format" = "csv",
+ "skip_lines" = "1"
);
"""
- assertTrue(Long.parseLong(hfCountWildcard[0][0].toString()) > 0)
qt_sql17 """
desc function
http(
- "uri" =
"hf://datasets/fka/awesome-chatgpt-prompts/blob/main/*.csv",
- "format" = "csv"
+ "uri" =
"${stableRemoteDataUrl}/awesome-chatgpt-prompts/61229c8c07ea12c6e219b2b2b728f23f2842901b/prompts.csv",
+ "format" = "csv",
+ "skip_lines" = "1"
);
"""
- // branch
qt_sql18 """
select * from
http(
- "uri" = "hf://datasets/stanfordnlp/imdb@script/dataset_infos.json",
+ "uri" =
"${stableRemoteDataUrl}/stanfordnlp-imdb/script/dataset_infos.json",
"format" = "json"
);
"""
qt_sql19 """
select * from
http(
- "uri" =
"hf://datasets/stanfordnlp/imdb@main/plain_text/test-00000-of-00001.parquet",
+ "uri" =
"${stableRemoteDataUrl}/stanfordnlp-imdb/main/plain_text/test-00000-of-00001.parquet",
Review Comment:
This doesn't add downloads, and this PR's P0 run shows no stall.
- The same files were downloaded before. On master, sql19, sql20 and the
second sql21 each read
`stanfordnlp/imdb@main/plain_text/test-00000-of-00001.parquet` (20.5 MB) from
huggingface.co, and the first sql21 (`*/*.parquet`) expands to all three
Parquet files of the repo (20.5 + 21.0 + 42.0 MB). That is about 145 MB per run
from HuggingFace. After this PR the four reads (`test_http_tvf.groovy:394-424`)
fetch about 82 MB from the regression OSS bucket.
- This PR's P0 ran them (build 1092831 at 6ed33949415): `test_http_tvf`
passed in 8.9 s, with sql19, sql20 and the two sql21 queries taking 1.8, 1.3,
1.7 and 1.3 s. The 39 P0 runs before it, with the HuggingFace version, took 149
to 170 s (median 151 s).
- The 900 s / 10 KB/s in the commit message is the author's workstation,
where the BE's traffic went through a local proxy; a direct GET of the same
object from that machine runs at about 600 KB/s. It is not a measurement of CI.
- The reads are bounded: each query is capped by `query_timeout` (900 s by
default,
`fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:1239`),
which is what ended the local run, and the FE size probe uses a 10 s connect
and 30 s read timeout
(`fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/manager/HttpUtils.java:180-181`).
P0 runs 14 suites in parallel, so a slow suite takes one worker, not the whole
run.
- The bucket isn't new to P0: `query_p0/test_parquet_dict` reads from it
(`regression-test/suites/query_p0/test_parquet_dict.groovy:23`) and passed its
last 400 P0 / cloud_p0 runs.
- A local fixture doesn't work for these files: an added file over 1 MB
fails the large-file check on master
(`.github/workflows/lfs-warning.yml:56-67`, `:91-97`), and a smaller substitute
would change the sql19 to sql21 goldens, which are identical on branch-4.1 and
branch-4.2.
So no change here; I've added the P0 numbers to the PR description.
##########
regression-test/suites/external_table_p0/tvf/test_http_tvf.groovy:
##########
@@ -151,12 +151,17 @@ suite("test_http_tvf", "p0") {
String httpServerHost = context.config.otherConfigs.get("httpTvfHost")
if (httpServerHost == null || httpServerHost.trim().isEmpty()) {
httpServerHost = context.config.otherConfigs.get("externalEnvIp")
- }
- if (httpServerHost == null || httpServerHost.trim().isEmpty()) {
- httpServerHost = InetAddress.getLocalHost().getHostAddress()
+ // A loopback or wildcard externalEnvIp would point a BE on another
host at itself.
+ if (httpServerHost == null || httpServerHost.trim().isEmpty()
+ || InetAddress.getByName(httpServerHost).isLoopbackAddress()
+ || InetAddress.getByName(httpServerHost).isAnyLocalAddress()) {
+ httpServerHost = InetAddress.getLocalHost().getHostAddress()
Review Comment:
The listener part doesn't hold, and the URL part is an existing limitation
with an override.
- `HttpServer.create(new InetSocketAddress("0.0.0.0", 0), 0)`
(`test_http_tvf.groovy:32`) is dual-stack on a JVM with IPv6. The JDK opens an
AF_INET6 channel, binds the IPv4 wildcard as `::`, and clears `IPV6_V6ONLY`
(OpenJDK 17: `ServerSocketChannelImpl.java:108-109`, `Net.java:559-567`,
`net_util_md.c:281-298`, `Net.c:267-277`). On JDK 17.0.19 the server listens as
`IPv6 *:<port>` and answers both `http://127.0.0.1:<port>/` and
`http://[::1]:<port>/`. With `-Djava.net.preferIPv4Stack=true` name lookups are
IPv4-only as well, so in neither mode can the listener and `getLocalHost()`
disagree on the address family.
- `getLocalHost()` returns an IPv6 address only if the host name has no IPv4
address, since the JDK puts IPv4 first while `java.net.preferIPv6Addresses` is
unset, or if the JVM sets that property. On P0 it returned `172.16.0.146`
(build 1092831: `Start local HTTP server for http tvf test:
http://172.16.0.146:45365`) and the suite passed.
`ai_p0/test_ai_structured_output` has used `getLocalHost().getHostAddress()`
unconditionally with the same listener and URL form
(`regression-test/suites/ai_p0/test_ai_structured_output.groovy:30`, `:50`,
`:55`) and passed its last 200 P0 / cloud_p0 runs.
- The brackets are missing for an IPv6 literal, but that predates this PR:
master already builds `http://${host}:${port}` from `httpTvfHost`,
`externalEnvIp` or this same `getLocalHost()` fallback
(`test_http_tvf.groovy:151-158` at 19dfb8711ae). A runner whose name resolves
only to IPv6 can set `httpTvfHost = "[addr]"`, which is used as given
(`test_http_tvf.groovy:151-152`).
No pipeline runs on a runner whose name resolves only to IPv6, so I'm
keeping the branch-4.x logic unchanged here.
--
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]
