Add support for reading/writing timestamps without timezone.

[sshkvar]

Creating PR from my forked repo as discussed #2282

[RussellSpitzer]

You can just put this right into the AssertThrows, we usually do this.

AssertThrows(errorMessage, IllegalArguementException.class, SparkUtil.TIMESTAMP_WITHOUT_TIMEZONE_ERROR,
  () -> code);

[RussellSpitzer]

Instead of using the rowToSqlValues function, it may be simpler to just do a spark.createDataset(values).write command. That way we don't have to worry about string building

[sshkvar]

I have moved insert Runnable and errorMessage to the AssertThrows

I would like to leave rowToSqlValues if you do not mind. From my point of view this is better to have pure sql in this test

[RussellSpitzer]

I just have a few little clean up nits, I think once these are done we should be good to go!

[sshkvar]

@RussellSpitzer I have pushed code changes for requested clean up nits

[RussellSpitzer]

wrong indentation here, .option should be indented as it's a continuation of spark.read.format and not another arg to assertThrows

[sshkvar]

you are right, fixed

[RussellSpitzer]

Tests running now!

[sshkvar]

Tests running now!

Added fix for Extra separation in import group before 'java.io.File' [ImportOrder]

[sshkvar]

added additional formatting fix for 'lambda arguments' has incorrect indentation level 12, expected level should be one of the following: 6, 8.

[RussellSpitzer]

@sshkvar for future reference you can use g build -x test -x integrationTest -x testSpark31 to run the checkstyle and other static analysis tests

[kbendick]

This is in AvroDataTest, but there is some code for handling additional Spark SQL conf options here that might be something worth copying:

iceberg/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java

Lines 171 to 206 in 25eaeba

	protected void withSQLConf(Map<String, String> conf, Action action) {
	SQLConf sqlConf = SQLConf.get();
	Map<String, String> currentConfValues = Maps.newHashMap();
	conf.keySet().forEach(confKey -> {
	if (sqlConf.contains(confKey)) {
	String currentConfValue = sqlConf.getConfString(confKey);
	currentConfValues.put(confKey, currentConfValue);
	}
	});
	conf.forEach((confKey, confValue) -> {
	if (SQLConf.staticConfKeys().contains(confKey)) {
	throw new RuntimeException("Cannot modify the value of a static config: " + confKey);
	}
	sqlConf.setConfString(confKey, confValue);
	});
	try {
	action.invoke();
	} finally {
	conf.forEach((confKey, confValue) -> {
	if (currentConfValues.containsKey(confKey)) {
	sqlConf.setConfString(confKey, currentConfValues.get(confKey));
	} else {
	sqlConf.unsetConf(confKey);
	}
	});
	}
	}
	@FunctionalInterface
	protected interface Action {
	void invoke();
	}
	}

Additionally, it might be good to unset this property after its use (so the configuration doesn't get mixed up with other tests now or in the future).

[sshkvar]

good point, I changed the code based on your recommendations

[sshkvar]

@sshkvar for future reference you can use g build -x test -x integrationTest -x testSpark31 to run the checkstyle and other static analysis tests

ok, thanks

[sshkvar]

Check failed

org.apache.iceberg.actions.TestDeleteReachableFilesAction24 > dataFilesCleanupWithParallelTasks FAILED
    java.lang.AssertionError: FILE_A should be deleted

Locally this test passed successfully. @RussellSpitzer Can we try to rerun this step?

[RussellSpitzer]

I can trigger a re-run, just pinged @karuppayya to take a look

[kbendick]

Nit: I think this is overly indented.

When continuing an expression on a second line, we usually indent 4 spaces (as opposed to the normal 2 spaces for changes in scope etc).

So I think that new SparkFixup… on this line should align the n of new with the second r from return above it (4 spaces in from the start of the word return).

[sshkvar]

I will fix it asap, where can I find code style guide?

[sshkvar]

I have pushed fix

[kbendick]

If you use IntelliJ, you can have it set up to point to the style guide and auto format it for you: http://iceberg.apache.org/community/#setting-up-ide-and-code-style

Otherwise, in general I believe the rules come from here: https://github.com/apache/iceberg/blob/master/.baseline/idea/intellij-java-palantir-style.xml

There are admittedly a number of these cases, so auto formatting seems like the best idea. If you don’t use IntelliJ, I think this command can be run from command line somehow but I’m not 100% sure.

Thanks for all the work on this so far. It’s very close!

[kbendick]

This is the plug-in that is used in general for your info. Specifically, this one would be the checkstyle one, but we also use error prone as well: https://github.com/palantir/gradle-baseline

[sshkvar]

Thanks a lot, I am using IntelliJ so it is really helpful

[kbendick]

Would it make sense to add this check into SparkSchemaUtil.convert?

[sshkvar]

I think no, because we need this check only in few places, while SparkSchemaUtil.convert used in a lot of places

[kbendick]

For reference, this is something just to consider and not a strong mandate or requirement to merge.

[kbendick]

As a follow up PR (in a separate PR either before or after this one is merged), particularly if you're looking for some more work to do to contribute to the project, you might explore if this combination of withSQLConf and the corresponding @FunctionalInterface can be abstracted into their own interface that one could mix into tests.

I'm not 100% sure how that would look, maybe an interface like ConfigurableTestSQLConf or something?

Again, just copying it for now is fine, but it would be nice to reduce the code duplication and make this easier for others to use in the future. Your exploration might find that it’s better to not do that (I’m more of a Scala developer myself and so to me it feels like a mixin). Something to think about for later though!

[sshkvar]

I am fully agree with you, it can be moved to separate interface with static method and placed in some general package like

@FunctionalInterface
public interface ConfigurableTestSQLConf {

  void invoke() throws IOException;

  static void withSQLConf(Map<String, String> conf, ConfigurableTestSQLConf action) throws IOException {
    SQLConf sqlConf = SQLConf.get();

    Map<String, String> currentConfValues = Maps.newHashMap();
    conf.keySet().forEach(confKey -> {
      if (sqlConf.contains(confKey)) {
        String currentConfValue = sqlConf.getConfString(confKey);
        currentConfValues.put(confKey, currentConfValue);
      }
    });

    conf.forEach((confKey, confValue) -> {
      if (SQLConf.staticConfKeys().contains(confKey)) {
        throw new RuntimeException("Cannot modify the value of a static config: " + confKey);
      }
      sqlConf.setConfString(confKey, confValue);
    });

    try {
      action.invoke();
    } finally {
      conf.forEach((confKey, confValue) -> {
        if (currentConfValues.containsKey(confKey)) {
          sqlConf.setConfString(confKey, currentConfValues.get(confKey));
        } else {
          sqlConf.unsetConf(confKey);
        }
      });
    }
  }
}

But this part better to do in separate PR, because other packages will be affected

[kbendick]

This looks good to me. Thank you @sshkvar!

[RussellSpitzer]

Thanks @sshkvar + @bkahloon ! This is a great contribution

Solves #2244