Hi, @justinmclean @tengqm @xunliu @jerryshao , could you plz help to see if this design is reasonable when you have some time.

I proposed a similar solution in PR #5688 @jerryshao thought that building command maps was too heavy. There is an outstanding PR to remove some of the if/else logic and use switch/cases instead. (PR #5793)

Yes. My favorite is the simple naive implementation using switch/case statements.
One key design consideration, in my view, is to make the code as straightforward as
possible. The key value of a software is not about its sophisticated class hierarchy design.
It is instead the business value for users and the elegant (simple) code base for developers.

I proposed a similar solution in PR #5688 @jerryshao thought that building command maps was too heavy. There is an outstanding PR to remove some of the if/else logic and use switch/cases instead. (PR #5793)

@justinmclean, @tengqm Can we combine both approaches(#5793 ) by using a switch/case structure for logical decisions, while also creating a Data class object to handle parameter validation and other logic operations?"

@justinmclean @tengqm Another concern is the frequent issues with changing error messages. The root cause of this lies in parameter validation not being performed before executing specific operations. As a result, methods like FullName and NameIdentifier.of end up throwing exceptions. Personally, I would like to combine Justin's switch/case logic(#5793 ) with a unified approach to argument validation.

Personally, I would like to combine Justin's switch/case logic(#5793 ) with a unified approach to argument validation.

100% support from my side.

Personally, I would like to combine Justin's switch/case logic(#5793 ) with a unified approach to argument validation.

I think it ok, The big logic branch uses switch/case, and The detailed logic (parameter validation) uses Data class object.

@justinmclean What's do you thnk?

My concern is that @jerryshao would prefer a different approach and it does add a lot of complexity, new methods and new objects. A better way, I think, would be to add a verify() method to commands and have that check arguments if needed. Note that only a couple of commands need to do this, not all of them. The CLI library does a lot of the work for us, so we don't need to duplicate what it does.

My concern is that @jerryshao would prefer a different approach and it does add a lot of complexity, new methods and new objects. A better way, I think, would be to add a verify() method to commands and have that check arguments if needed. Note that only a couple of commands need to do this, not all of them. The CLI library does a lot of the work for us, so we don't need to duplicate what it does.

Hi @justinmclean , Can I draft a version first? I’d like to build on #5793 and submit a draft PR to show how it works.

This is what is would look like. Add a verify method to the base Command class and a few common checkers.

  public Command verify() {
    return this;
  }

  public Command verifyTableName(String metalake, String catalog, String schema, String table) {
    if (metalake == null) {
      throw new IllegalArgumentException("Missing metalake name");
    }
    if (catalog == null) {
      throw new IllegalArgumentException("Missing catalog name");
    }
    if (schema == null) {
      throw new IllegalArgumentException("Missing schema name");
    }
    if (table == null) {
      throw new IllegalArgumentException("Missing table name");
    }
    return this;
  }

(we may not need to check for metalake)

in a command that needs to check table names add this:

  @Override
  public Command verify() {
    return verifyTableName(metalake, catalog, schema, table);
  }

When calling the command chain verify and handle together:

      newListColumns(url, ignore, metalake, catalog, schema, table).verify().handle();

Yep there is no need to check for metalake in the above code, as that is already done, and there is probably no need to check for catalog as name must have a value at this point, which means catalog is always set, so that method only needs schema and table passed to it to check.

Yep there is no need to check for metalake in the above code, as that is already done, and there is probably no need to check for catalog as name must have a value at this point, which means catalog is always set, so that method only needs schema and table passed to it to check.

@justinmclean Should we throw an exception in the verify function? Or give details that are missing.

We would need to throw an exception so that no code in handle is called. The exception message can give the reason why.

We would need to throw an exception so that no code in handle is called. The exception message can give the reason why.

hi, @justinmclean Adding the verify to Command I have a few concerns

1. The scope of the change is too large, we need modify all subclass commands.
2. The potential for excessive duplication in validation logic.
3. The need to wrap the handle method in a try/catch block each time it is invoked.

@shaofengshi could you plz help to see?

You do not have to modify all subclasses. Remember, the CLI library does a lot of the work for you before you get to this point, and there is no need to check for things that have already been checked. There should not be excessive duplication if you place common validation in methods in the Command base class. If the exception is an issue, you could get around that by using System.err.println and System.exit(-1)?

hi, @justinmclean Has the modification of the Command been completed? Specifically, has the verify method been added, or is someone else currently working on it? If not, could this task be assigned to me?

BTW, can this PR #5793 merged?

It needs to be reviewed before it can be merged, I can't review or merge it as I wrote the code.

It needs to be reviewed before it can be merged, I can't review or merge it as I wrote the code.

Got it. The modification of Command depends on #5793. This PR is highly valuable, and I hope it gets merged soon.