Allow to configure insert into Hive partition via configuration property

[ssheikin]

Allow to set default value for hive.insert_existing_partitions_behavior property

[kokosing]

@electrum Would you please take a look?

[findepi]

Will we deprecate immutable partitions property?

[ssheikin]

No. immutable partitions = true is used to protect setting hive.insert-existing-partitions-behavior to APPEND state via session properties.

[losipiuk]

You could extend hive.insert-existing-partitions-behavior with FORCE_ERROR (name subject to discussion) which behaves as ERROR, but also be non overridable by session property.

Though personally I feel that having separate properties is easier to understand.

[ssheikin]

For some reason immutable partitions=true allows setting insert-existing-partitions-behavior=OWERWRITE that's why it is not an option. Apart from that deprecating breaks backwards compatibility and requires a longer discussion.
Won't do.

[losipiuk]

If I read the code correctly now if we specify hive.immutable-partitions=true and NOT specify hive.insert-existing-partitions-behavior the Presto will fail to start.
This is valid setup today. To be consistent with what we have right now we should default hive.insert-existing-partitions-behavior to ERROR if hive.immutable-partitions is true.

[losipiuk]

Also can you add test for above.

[ssheikin]

Good point!

I've tried to keep old logic of insert-existing-partitions-behavior calculation, when hive.insert-existing-partitions-behavior is NOT specified:

• Calculate default value during @PostConstruct phase

    private Optional<InsertExistingPartitionsBehavior> insertExistingPartitionsBehavior = Optional.empty();

    public InsertExistingPartitionsBehavior getInsertExistingPartitionsBehavior()
    {
        return insertExistingPartitionsBehavior.orElseThrow(() -> new NoSuchElementException("insertExistingPartitionsBehavior was not set during presto initialization"));
    }

    @Config("hive.insert-existing-partitions-behavior")
    @ConfigDescription("Default value for insert existing partitions behavior")
    public HiveConfig setInsertExistingPartitionsBehavior(InsertExistingPartitionsBehavior insertExistingPartitionsBehavior)
    {
        this.insertExistingPartitionsBehavior = Optional.of(requireNonNull(insertExistingPartitionsBehavior, "insertExistingPartitionsBehavior is null"));
        return this;
    }

    @PostConstruct
    public void validate()
    {
        if (insertExistingPartitionsBehavior.isEmpty()) {
            insertExistingPartitionsBehavior = getInsertExistingPartitionsBehaviorForBackwardCompatibility();
        }
        else {
            InsertExistingPartitionsBehavior.validate(insertExistingPartitionsBehavior.get(), immutablePartitions);
        }
    }

    private Optional<InsertExistingPartitionsBehavior> getInsertExistingPartitionsBehaviorForBackwardCompatibility()
    {
        return Optional.of(immutablePartitions ? ERROR : APPEND);
    }

but seems it's not good idea from frameworks' point of view (even not looking to even more complicated logic), because tests are not configured to maintain such complex logic TestHiveConfig#testDefaults() does not invoke @PostConstruct method.

• However it is possible to move logic from @PostConstruct to getter (code becomes not that clear).

    private Optional<InsertExistingPartitionsBehavior> insertExistingPartitionsBehavior = Optional.empty();

    public InsertExistingPartitionsBehavior getInsertExistingPartitionsBehavior()
    {
        if (insertExistingPartitionsBehavior.isEmpty()) {
            insertExistingPartitionsBehavior = getInsertExistingPartitionsBehaviorForBackwardCompatibility();
        }
        return insertExistingPartitionsBehavior.orElseThrow(() -> new NoSuchElementException("insertExistingPartitionsBehavior was not set during presto initialization"));
    }

    private Optional<InsertExistingPartitionsBehavior> getInsertExistingPartitionsBehaviorForBackwardCompatibility()
    {
        return Optional.of(immutablePartitions ? ERROR : APPEND);
    }

    @Config("hive.insert-existing-partitions-behavior")
    @ConfigDescription("Default value for insert existing partitions behavior")
    public HiveConfig setInsertExistingPartitionsBehavior(InsertExistingPartitionsBehavior insertExistingPartitionsBehavior)
    {
        this.insertExistingPartitionsBehavior = Optional.of(requireNonNull(insertExistingPartitionsBehavior, "insertExistingPartitionsBehavior is null"));
        return this;
    }

    @PostConstruct
    public void validate()
    {
        insertExistingPartitionsBehavior.ifPresent(v -> InsertExistingPartitionsBehavior.validate(v, immutablePartitions));
    }

@kokosing I see three possible solutions:

1. apply solution proposed by @losipiuk which changes default value hive.insert-existing-partitions-behavior to ERROR, which breaks backward compatibility when immutablePartitions=false (in this case insert-existing-partitions-behavior was set to APPEND).
2. Break backwards compatibility for one of valid setups today: hive.immutable-partitions=true and hive.insert-existing-partitions-behavior is NOT specified.
3. Implement logic in getter.

What solution would be better?

[ssheikin]

I'll implement solution 3.

[findepi]

@zaz968m there are conflicts now. can you please rebase?

[losipiuk]

@electrum can you review the docs?

[losipiuk]

taking back LGTM for now until last piece of discussion is resolved

.

[ssheikin]

@losipiuk @kokosing all comments are addressed.

[losipiuk]

nit: you can use concrete type here

[ssheikin]

It requires unnecessary explicit casting. I think it does not worth adding additional characters in this case.
However I would be glad to substitute method's body with shorter and more understandable implementation, but I can't find one.

[ssheikin]

@electrum all comments are addressed