FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(session): add tableExists and deregisterTable to SessionContext by devoopsman45 · Pull Request #102 · apache/datafusion-java · GitHub

  1. apache
  2. datafusion-java

Repository navigation

feat(session): add tableExists and deregisterTable to SessionContext - #102

Merged
coderfender merged 4 commits into
apache:mainfrom
devoopsman45:feat/session-table-exists-deregister
Jun 9, 2026
ConversationCommits (4)ChecksFiles changed
Merged

feat(session): add tableExists and deregisterTable to SessionContext#102
coderfender merged 4 commits into
apache:mainfrom
devoopsman45:feat/session-table-exists-deregister

Conversation

devoopsman45 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

SessionContext can let a caller register tables but there is no way to

Check whether a table name is already registered
Remove a registered table from the session

This makes it tough to write safer code while registering

What changes are included in this PR?

  • SessionContext.tableExists(String name) — returns true if a table
    with that name is registered in the session

    • SessionContext.deregisterTable(String name) — removes a registered
      table; no-op if the name is not found

    Both are thin JNI wrappers over DataFusion's existing
    SessionContext::table_exist and SessionContext::deregister_table
    on the Rust side.

Are these changes tested?

Yes. SessionContextTableRegistrationTest covers:

  • tableExists returns false for an unregistered table
  • tableExists returns true after registerCsv
  • deregisterTable removes a registered table
  • deregisterTable is a no-op for an unregistered table
  • Both methods throw IllegalStateException on a closed context

-->

Are there any user-facing changes?

Yes — two new public methods on SessionContext. Additive only, no
breaking changes.

hiteshkumardasika commented Jun 6, 2026

Copy link
Copy Markdown

@coderfender Can you please review?

coderfender reviewed Jun 6, 2026
View reviewed changes

coderfender 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you @devoopsman45 , left some comments. Also, is a noop standard across df bindings when a table does not exist?

Comment thread core/src/main/java/org/apache/datafusion/SessionContext.java Outdated
Comment thread core/src/main/java/org/apache/datafusion/SessionContext.java
Comment thread core/src/test/java/org/apache/datafusion/SessionContextTableRegistrationTest.java Outdated
Comment thread core/src/test/java/org/apache/datafusion/SessionContextTableRegistrationTest.java Outdated
Comment thread native/src/lib.rs Outdated
Comment thread native/src/lib.rs Outdated

devoopsman45 commented Jun 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@coderfender Can you please review?

Thank you @devoopsman45 , left some comments. Also, is a noop standard across df bindings when a table does not exist?

@coderfender
Yes, Datafusion Rust API deregister_table returns Result<Option<...>> i.e. it is returning an Option indicating it is designed to be idempotent. Hence, on the Java side, treating it as no-op makes sense.

Please let me know your thoughts. Thanks

coderfender reviewed Jun 6, 2026
View reviewed changes

coderfender 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@devoopsman45 , @hiteshkumardasika Thank you for the changes. I believe we are close to getting this merged.

Comment thread core/src/main/java/org/apache/datafusion/SessionContext.java
}
if (name == null) {
throw new IllegalArgumentException("tableExists name must be non-null");
}

coderfender Jun 6, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does the native code throw an exception when a null invariant is passed ?

devoopsman45 Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If null is passed from Java, the Java native call spits out a null JString. Then in Rust, env.get_string() on a null JString would return a JNI error, which propagates out, and
try_unwrap_or_throw catches it and throws a Java RuntimeException or DataFusionException back. I believe that is not a right Exception to be thrown.

coderfender Jun 7, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for the explanation. Should we probably set a more specific exception handling mechanism for future enhancements to follow through? ( we can do that in a follow up pr though)

devoopsman45 Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, Thanks. I will create a followup Enhancement for this

Comment thread native/src/lib.rs
coderfender reviewed Jun 7, 2026
View reviewed changes
Comment thread core/src/main/java/org/apache/datafusion/SessionContext.java

private void checkOpen() {
if (nativeHandle == 0) {
throw new IllegalStateException("SessionContext is closed");

coderfender Jun 7, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit : could probably make the method name a little more robust like 'checkOpenSessionContext' or so to keep users from guessing

coderfender Jun 7, 2026

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could we perhaps use option to make the code more functional and let the callers handle exception/ success accordingly?

devoopsman45 Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the feedback. This file in general has a throw on closed sort of pattern. If optional is returned, it would force every caller to .orElseThrow() paradigm, which seems a bit noisy for private helper. We can probably do this as a seperate enhancement if needed.

devoopsman45 Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@coderfender Fixed the method naming. Thank you

coderfender commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Thank you @devoopsman45

devoopsman45 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@coderfender Thank you for review. Ready to merge whenever you are. Here are the followup Issues I am going to create as disucssed above in the comments.

  1. Refactor — apply checkOpenSessionContext() and unwrap_context() consistently across all existing methods in SessionContext.java and lib.rs
  2. Enhancement — more specific exception type when null is passed as a table name through JNI

coderfender commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Sure . Feel free to add the issues on this PR to track lineage

coderfender merged commit 60351ba into apache:main Jun 9, 2026
5 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

coderfender coderfender approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(session): add tableExists and deregisterTable to SessionContext

3 participants

Back | FazBrowse Home | New Git URL