Skip to content

fix(cli): report correct entity type for missing property - #1621

Open
jx2lee wants to merge 2 commits into
apache:mainfrom
jx2lee:jx2lee/missing-property-report
Open

fix(cli): report correct entity type for missing property#1621
jx2lee wants to merge 2 commits into
apache:mainfrom
jx2lee:jx2lee/missing-property-report

Conversation

@jx2lee

@jx2lee jx2lee commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • properties get always reported "on namespace" in its error message even when the target was a table, since the message was hardcoded instead of using the actual entity type.
  • Switched to fmt.Errorf with get.Type so the message reflects "namespace" or "table" correctly.
  • Add unit test verifying properties get reports correct entity type on missing property

Test plan

go test ./cmd/iceberg/

Ran against a local hadoop catalog, requesting a non-existent property:

Before

$ iceberg properties get table ns1.t1 nonexistent_prop
could not find property nonexistent_prop on namespace ns1.t1   # wrong: says "namespace" for a table

After

$ iceberg properties get table ns1.t1 nonexistent_prop
could not find property nonexistent_prop on table ns1.t1       # correct

$ iceberg properties get namespace ns1 nonexistent_prop
could not find property nonexistent_prop on namespace ns1      # unchanged, still correctn	

@jx2lee
jx2lee force-pushed the jx2lee/missing-property-report branch from ba03430 to 089af7e Compare July 31, 2026 11:17
@jx2lee
jx2lee marked this pull request as ready for review July 31, 2026 11:38
@jx2lee
jx2lee requested a review from zeroshade as a code owner July 31, 2026 11:38

@tanmayrauth tanmayrauth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is correct — validateEntityType guarantees get.Type is only ever "namespace" or "table", so the interpolated message is safe. Couple of nits inline.

Comment thread cmd/iceberg/properties_test.go Outdated
panic("UpdateNamespaceProperties must not be called")
}

func TestRunPropertiesGetNamespaceNotFound로(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name TestRunPropertiesGetNamespaceNotFound로 has a stray Korean character on the end — looks like an accidental keystroke. It compiles and runs (Go allows Unicode identifiers), but it shows up as garbage in CI logs and when grepping test names, and it's inconsistent with the sibling TestRunPropertiesGetTableNotFound. Rename to TestRunPropertiesGetNamespaceNotFound.

Comment thread cmd/iceberg/main.go Outdated
output.Error(fmt.Errorf("could not find property %s on %s %s", get.PropName, get.Type, get.Identifier))
osExit(1)

return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return here is unreachable — osExit is os.Exit in prod (never returns) and panics in tests, and this is already the end of the case. Harmless, but you can drop it.

jx2lee added 2 commits August 1, 2026 01:47
- and modfiied os.Exit to osExit for testcode
@jx2lee
jx2lee force-pushed the jx2lee/missing-property-report branch from 089af7e to e48339a Compare July 31, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants