Add Eclipse Collections as a dependency.

[itohro]

• Add Eclipse Collections as a dependency
• Add a flag to generate asEcList() method
• Made Selectors extend EC Function so that uses have a choice to use Reladomo selectors with Eclipse Collections

[gs-rezaem]

This is a good start. Did you test it with JDK 8/EC 8? It's simple enough that it should work.

However, this is going to add confusion to our status of GSC/EC support. I'd rather have all the changes in one changeset, and do a release with a 1 year GSC deprecation notice.

[itohro]

Hi @gs-rezaem, thanks for your comment.

I'll test it with JDK 8 and EC 8.

Just to double check what you mean by "I'd rather have all the changes in one changeset", is it to replace all internal GSC dependencies with EC in reladomo?

[gs-rezaem]

Actually, the internal dependencies don't have to be in the first commit. It's important that all public API's have both a EC and GSC variant (look at in() methods, for example). The GSC variant should be marked deprecated with a 1 year date in the future marking its removal.

We can do the internal dependencies after that.

[itohro]

I'm still in middle of updating this, but I've got one design question. Please let me know how you think.

There are places I couldn't create synonymous variants with the same name (e.g. ByteArraySet, MappedAttribute#getParentSelector() etc.). For those cases, I'm thinking to create classes or methods named ByteArrayEcSet or MappedAttribute#getParentEcSelector(). This may be a bit odd because these names will persist for backward compatibility even after GCS removal. Not sure if you like this approach. Please let me know how you think.

[gs-rezaem]

I don't think you should use a new method or class in those two cases. For ByteArraySet, extend the EC UnifiedSetWithHS and implement GSC MutableMap. For getParentSelector, introduce a new Reladomo interface that extends both EC and GSC Function. The idea is to keep it compile compatible with the current version.

[itohro]

Hi @gs-rezaem , thanks for your advice earlier.

I got similar cases where we need to create new reladomo interface or class that implements or extends both GSC and EC. However for these cases, new classes cannot compile due to the conflicts in method signature. Below are the examples.

https://gist.github.com/itohro/7e1a146058168ddc14a154f212640c1c

What's your preference to handle these cases?

[gs-rezaem]

I think those Constant set classes are very unlikely to be used as a GSC interface. You can forget the GSC interface for those, as it's unlikely to cause a serious backward incompatibility.

[itohro]

I've added a new commit to deprecate GSC variant APIs and add EC variant APIs.

Please let me know for any comments. I'm happy to squash the commits if you prefer.

[gs-rezaem]

Given the size of this, I think we should push this to a new branch (call it ec). Please commit this in goldmansachs/reladomo in a new branch.

I'm a bit worried about omissions, which is hard to see in a diff view.

One way we may be able to show we're complete is to remove the dependency on the gsc impl jar.

[gs-rezaem]

One more thing: in your code, you're converting from EC to GSC for internal storage. We need to reverse that.

[itohro]

One way we may be able to show we're complete is to remove the dependency on the gsc impl jar.

I left GSC UnifiedSet as is in ConstantStringSet that makes it still depends on gsc impl. Shall we replace that with EC UnifiedSet instead?

[itohro]

Also I find number of public methods take GSC impl in the signature. One example is AggregateData#setNameToPositionMap(ObjectIntHashMap map).

Shall we change those methods to take interface instead (same case for EC variants)?

[itohro]

Sorry for continuous comments. I find another place that requires GSC impl code. I don't think we can get rid of GSC impl dependency here.

In AggregateList.java, this method needs to initialize IntHashSet.

    public MutableIntSet getAttributeAsGscIntSet(String attributeName)
    {
        MutableIntSet result = new IntHashSet(this.size());
        for (int i = 0; i < this.size(); i++)
        {
            result.add(this.get(i).getAttributeAsInt(attributeName));
        }
        return result;
    }

[gs-rezaem]

ConstantStringSet and others like it: yes, replace.
AggregateData#setNameToPositionMap(ObjectIntHashMap map) : yes, replace with interface.
In places where we construct something, we'll have to leave it for now.

[itohro]

Closing this PR. I'll raise a new PR from my personal account.