What Problem Does This PR Solve?

Today LanceSinkFactory marks dataset_path and namespace_type as required options, but "required" only means "must resolve to some value" — since both options carry non-blank defaults (/test.lance and dir), a user can still explicitly configure dataset_path = "" or namespace_type = " " and the factory happily accepts it, because an explicit blank string is a present value, not an absent one. That blank value then flows straight into the Lance runtime as a real (and useless) path/namespace type. This PR closes that gap by attaching a notBlank value constraint to both options, so an explicitly blank/whitespace-only value now fails fast at config-validation time instead of surfacing later as a confusing runtime error from Lance itself.

1. Code Change Review

1.1 Core Logic Analysis

Change: LanceSinkFactory.optionRule() replaces

.required(LanceCommonOptions.KEY_DATASET_PATH, LanceCommonOptions.KEY_NAMESPACE_TYPE)

with

.required(
        LanceCommonOptions.KEY_DATASET_PATH,
        LanceCommonOptions.KEY_NAMESPACE_TYPE,
        notBlank(LanceCommonOptions.KEY_DATASET_PATH),
        notBlank(LanceCommonOptions.KEY_NAMESPACE_TYPE))

Correctness analysis: I traced this against the framework rather than taking the PR description at face value.

• OptionRule.java (unchanged by this PR — verified with git log on that file across the PR range) already has a purpose-built overload required(Option option1, Option option2, Condition condition1, Condition... conditions), whose own Javadoc explicitly exists so that required(opt1, opt2) still resolves unambiguously to the plain required(Option[]) overload. This PR is simply invoking existing, already-reviewed framework infrastructure correctly — it does not touch OptionRule, Condition, or ConditionEvaluators at all, so there is no risk of breaking any other connector's use of the same builder.
• ConditionEvaluators.NOT_BLANK's evaluator is v instanceof String && !((String) v).trim().isEmpty(), and it evaluates against config.get(condition.getOption()) — which is ReadonlyConfig.get(Option), the same accessor that fills in the option's default when the key is absent from the raw config map. That means: an omitted key resolves to the default (/test.lance, dir), which is non-blank and passes; an explicitly blank/whitespace value resolves to that literal string, which is blank and now fails. This is exactly what the PR description claims, and I verified it by reading the evaluator rather than trusting the description.
• namespace_id (KEY_NAMESPACE_ID, default "") is deliberately left out of the notBlank treatment, consistent with the PR's stated intent that it "remains optional with its empty-string default" — correct, since forcing non-blank on a field whose own default is blank would make the default itself invalid.
• This is a single, self-contained change to one factory's option rule with no engine, checkpoint, serialization, or cross-connector surface — no runtime-path diagram is needed.

1.2 Compatibility Impact

Partially incompatible, by design and correctly scoped. A config that previously set dataset_path or namespace_type to an explicit empty/whitespace string will now fail validation where it previously succeeded (silently, with a useless path). Any config that omits these keys, or sets them to a real non-blank value, is completely unaffected. This is a deliberate tightening of validation for a case that was never doing anything useful, not a behavioral change for any working configuration — an acceptable and well-documented incompatibility.

1.3 Performance / Side-Effect Analysis

None. This only adds a cheap string-trim/blank check at config-validation time, before any Lance I/O happens.

1.4 Error Handling and Logging

No new issues found. The failure path reuses the existing ConfigValidator/OptionValidationException machinery, whose message includes the offending option's key (confirmed via ConfigValidator.java's constraint-violation message construction, which calls constraint.toString() — not modified by this PR), so the new tests' assertTrue(error.getMessage().contains(key)) assertions are meaningfully checking something, not vacuously true.

2. Code Quality Assessment

2.1 Coding Standards

Clean, minimal diff. No new classes, no new production methods needing class-level Javadoc — this is a one-line behavioral change inside an existing method. No banned symbols, no System.out.println, no inline single-line Javadoc.

2.2 Test Coverage and Test Stability

Coverage is thorough for the size of the change: nonblank-accepted, whitespace-preserved-not-trimmed, all six blank/whitespace variants ("", " ", \t, \n, \r, mixed) rejected for both options via @ParameterizedTest, omitted-option-uses-default for both options individually and together, and namespace_id's optional/blank-preserving behavior. All assertions are precise (exact expected string equality, exception type, and message-contains-key), not loose existence checks.

I checked specifically for flaky-test patterns: no Thread.sleep, no fixed ports, no shared mutable state, no timing assertions, no network/filesystem I/O (ConfigValidator is exercised directly against an in-memory ReadonlyConfig, never starting Lance). Rating: Stable.

2.3 Documentation Updates

Both docs/en/connectors/sink/Lance.md and docs/zh/connectors/sink/Lance.md gained a one-line note under dataset_path and namespace_type describing the new non-blank constraint and the preserved default, in both languages, and the wording matches the actual validated behavior (I checked the evaluator, not just the prose). No English/Chinese content leaked into the wrong file.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. This is the smallest possible change that closes the actual gap (an explicit-blank value slipping through "required"), reusing existing, already-established validation infrastructure (Conditions.notBlank + the matching OptionRule.required(...) overload) exactly as it is documented to be used elsewhere in the codebase, rather than hand-rolling ad hoc validation in the factory or the sink.

3.2 Maintainability

High. The change is entirely declarative (adding a condition to an existing builder call), easy to read, and the test suite documents the exact intended behavior for future maintainers.

3.3 Extensibility

Not applicable — this is a leaf-level connector option-rule change with no extension surface implications.

3.4 Historical-Version Compatibility

No checkpoint, state, serialization, or protocol involvement whatsoever; this only affects config validation at job-submission time. No upgrade-path or restore-path concern.

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge

I don't have any blockers or non-blocking suggestions for this PR — it's a small, correctly-scoped, well-tested, well-documented improvement that reuses existing framework infrastructure exactly as intended. As a reminder, my account here is comment-only and this review does not itself satisfy branch protection; a committer with write access still needs to give the actual merge approval.

Overall assessment: nothing to change. Nice, tight PR.

+1 LGTM