Skip to content

Conversation

amaliujia
Copy link
Collaborator

@amaliujia amaliujia commented Jul 27, 2024

Description of changes

[FOLLOW-UP] Format Spark Integration code by javafmtAll.

@amaliujia
Copy link
Collaborator Author

I also tried to run build/sbt javafmtAll and it seemed to update other files style as well.

@amaliujia
Copy link
Collaborator Author

@cloud-fan

@jaceklaskowski
Copy link
Contributor

Can you just leave the description of the changes alone? 🙏 Makes git log so terrible to look at 😢

override def listTables(namespace: Array[String]): Array[Identifier] = ???
override def listTables(namespace: Array[String]): Array[Identifier] = {
if (namespace.length > 1) {
throw new ApiException("Unity Catalog does not support nested namespace.")
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. "nested namespaces" (plural)
  2. Can you add what the namespace is in the exception
Nested namespaces are not supported: [namespace]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

if (namespace.length > 1) {
throw new ApiException("Unity Catalog does not support nested namespace.")
}
val response: ListTablesResponse = tablesApi.listTables(this.name, namespace.head, 0, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about introducing local variables with names for the input arguments, i.e. catalogName, schemaName, maxResults and pageToken? Java doesn't support named arguments unfortunately. I think it would make code comprehension a bit better (unless you're in IntelliJ IDEA and gets all this for free 😉 )

A bit weird to see null among the input arguments, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah I see. Sure done. This seems to be more readable.

@amaliujia
Copy link
Collaborator Author

Can you just leave the description of the changes alone? 🙏 Makes git log so terrible to look at 😢
Not sure if I understand this comment but tried to address it.

@cloud-fan
Copy link
Collaborator

This is hard to review because it also changes the indentation. Please don't do so as this project does not share the same code style standard with Apache Spark.

@amaliujia
Copy link
Collaborator Author

Screenshot 2024-07-28 at 9 52 14 PM

@cloud-fan the main page documentation says to format the code by build/sbt javafmtAll

@amaliujia amaliujia changed the title Support SHOW TABLES command for Spark [FOLLOW-UP] Format Spark Integration code by javafmtAll Jul 29, 2024
@cloud-fan cloud-fan merged commit 0de24a0 into unitycatalog:main Jul 29, 2024
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.

3 participants