morningman opened a new pull request, #66324:
URL: https://github.com/apache/doris/pull/66324
### What problem does this PR solve?
Issue Number: #65185
Related PR: #66004
Problem Summary:
Now that catalogs go through the connector/filesystem SPIs, fe-core and
fe-common no
longer have a reason to compile against hadoop, but a handful of source
imports were
still left over from the pre-SPI code. This removes them.
Scope is deliberately narrow: **source imports only**. The hadoop pom
dependencies stay
exactly as they are, because they remain reachable at runtime —
`ranger-plugins-common`
inherits from `org.apache.hadoop.conf.Configuration`, `hive-exec` supplies
the UDF base
class that `CREATE FUNCTION` resolves, and `fe-kerberos` uses
`UserGroupInformation`.
Dropping the jars is a separate, larger question and is not attempted here.
After this PR, `grep -rn "^import org.apache.hadoop" fe/fe-core/src
fe/fe-common/src`
returns exactly one line: `RangerHiveAuditHandler`, which is a sanctioned
exception. The
type is imposed by Ranger's own API, and `ranger-plugins-common` puts hadoop
on the
classpath regardless, so removing that import would remove no dependency. It
is
documented as such in the code rather than worked around.
The six commits are independent steps and are easiest to review one at a
time:
| Commit | What moves |
|---|---|
| `4314ad2` | Dead and trivial imports: delete `CatalogConfigFileUtils` (no
caller; `fe-filesystem-hdfs*` already carries the port) and
`LocationPath.getTempWritePath` (no caller); inline two hadoop constants that
were plain strings; document the Ranger exception |
| `29649e2` | `hadoop-huaweicloud` moves from fe-core to the
`fe-filesystem-obs` plugin, together with the `huawei-obs-sdk` repository
declaration. The only FE reference is the `Class.forName` probe in
`ObsFileSystemProperties`, which must resolve against that plugin's classloader
to report the truth |
| `90339cd` | `FileSplitter` takes a new fe-core `FileBlockLocation` instead
of hadoop's `BlockLocation`. Only `getOffset`/`getLength`/`getHosts` were ever
read |
| `c55800a` | `LocationPath.toStorageLocation()` returns the existing
`org.apache.doris.filesystem.Location` instead of a hadoop `Path`; `getPath()`
is deleted in favour of the `fsIdentifier` the class already computes. **See
the behaviour-change note below** |
| `592e8aa` | The Azure OAuth2 backend map moves from `StorageAdapter` into
`fe-filesystem-azure`, which is where the legacy `AzureProperties` owned it.
This deletes ~120 lines of fe-core code that turned out to be unreachable |
| `464efa4` | `StageUtil`'s `GlobExpander`/`GlobFilter` use is ported to a
package-private `GlobPatterns`, compiled with re2j — the same engine hadoop's
`GlobPattern` uses, and already a declared fe-core dependency |
Two notes that are easy to miss on review:
- `hadoop.fs.GlobFilter` reads like a wildcard predicate but its constructor
is also a
validator: it rejects `a[b`, `a{b`, a trailing backslash and `[z-a]`, and
`analyzeGlob`
surfaces that as a `DdlException`. Porting only the predicate would have
accepted those
globs and quietly listed an unintended object-store prefix instead of
failing the
statement, so the validation is ported with it.
- `StorageAdapter.getHadoopStorageConfig()` had no caller anywhere in the
tree; the only
consumer was the adapter's own Azure OAuth2 arm. That map is still
load-bearing (BE
routes Microsoft Fabric OneLake locations to `FILE_HDFS`, and
`hdfs_builder` feeds every
entry into its JNI hadoop builder), so it is preserved key-for-key rather
than dropped —
it just lives in the Azure plugin now.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Unit tests were added where the code being moved had no coverage:
`StageGlobTest` (glob prefix derivation, brace expansion and each
rejection — `analyzeGlob`
had none at all), the Azure OAuth2 backend map in
`AzureFileSystemPropertiesTest` (also
none), and
`FileSplitterTest.testNullBlockLocationsSplitLikeOneWholeFileBlock` (the null
path is the one `TVFScanNode` actually takes and it was uncovered).
Equivalence for the glob port was established differentially against the
hadoop classes
while they are still on the test classpath — their javadoc examples,
realistic stage
globs, malformed inputs and 20k fuzzed strings, comparing results,
exception types and
exception messages. That run caught a missing `Pattern.DOTALL` flag, which
never affects
matching here but does change the syntax-error text the user sees. The
harness itself is
not committed, since depending on hadoop is the thing being removed; the
committed test
pins the same behaviour with literals.
Each behaviour-preserving claim was mutation-checked (removing the glob
validation,
making `{` stop counting as a wildcard, reverting the Azure arm to the
plain map,
applying the hadoop dump to both auth types, halving the synthetic block
length) — every
mutation fails exactly the test that guards it.
Verified per commit: full reactor `package -DskipTests` with the build
cache disabled,
BUILD SUCCESS; `checkstyle:check` on each changed module, 0 violations.
No regression tests: the paths involved need a cloud-mode cluster (COPY
INTO) or a live
Azure/OneLake account, neither of which is reachable here. Azure OAuth2
has no existing
regression coverage either.
- Behavior changed:
- [ ] No.
- [x] Yes.
In `c55800a`, the path Doris sends to the BE is no longer rewritten on the
way out.
Routing it through hadoop's `Path` used to collapse repeated slashes, drop
a trailing
slash, resolve `.` and `..` segments, and turn an authority-less location
from
`scheme://x` into `scheme:/x`. None of that is wanted for object storage,
where `a//b`
and `a/b` name different objects and collapsing them rewrites the key.
This follows what
Trino did when it replaced hadoop `Path` with
`io.trino.filesystem.Location`, whose
javadoc states it "does not follow the format rules of a URI or URL" and
whose tests pin
slash preservation deliberately. Doris already had the equivalent type
(`org.apache.doris.filesystem.Location`), so this reuses it rather than
adding one.
Relatedly, `fsName` for an authority-less location is now `hdfs://`
instead of the
literal string `hdfs://null` that hadoop produced.
Two smaller consequences worth flagging: the Azure OAuth2 backend map is
now immutable
(it was a mutable `HashMap`; every reachable consumer only reads it), and
`fe-filesystem-azure` now bundles hadoop-common the way
`fe-filesystem-hdfs`, `-jfs` and
`-oss-hdfs` already do, which grows that plugin zip.
- Does this need documentation?
- [x] No.
--
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]
