On Fri, 18 Aug 2017 19:52:01 +0200, Gilles wrote:
On Fri, 18 Aug 2017 11:34:54 -0600, Gary Gregory wrote:
To be clear, you are then OK with simply adding the two put() methods to
CSVRecord? Option 1 in my eariler message.

Actually, I think that it is not OK to not acknowledge the
breaking nature of the proposal.
As Simon noted you might want to consider that such a change
warrants a major release, or not if you _decide_ that the
project never promised immutability (despite what the source
says through self-documentation).

I'm not opposing a decision to break, if that's your preferred
option (despite the technical arguments against what I proposed
have not been substantiated IIUC).

By the above paragraph I mean that I don't remember reading why
the following (pseudo-code) was not a good option:
---CUT---
CSRecord rec = readRecord(source);
rec = rec.copyAndReplace(new int[] {1, 4},
                         new String[] {"Change", "this"});
---CUT---

Gilles


In any cases, a mutable subclass is not a solution IMO.

Not sure I'm clear enough... :-)
Gilles

Gary

On Fri, Aug 18, 2017 at 10:05 AM, Gilles <gil...@harfang.homelinux.org>
wrote:

On Fri, 18 Aug 2017 09:36:06 -0600, Gary Gregory wrote:

Please see branch CSV-216 for a KISS implementation that uses a
CSVMutableRecord subclass.


I don't think anyone gains anything through this subclassing.

A client can no longer assume that an instance of "CSVRecord" is
immutable and will have to make a defensive copy or an "instanceof"
check (that will be obsolete/broken whenever the hierarchy is
modified).

Better assume a functionally breaking change and add the methods
to class "CSVRecord"...

Gilles



I do not believe this feature warrants creating interfaces or
framework-like code. I do not believe we need to start leaning the
JDBC-way.

Gary

On Thu, Aug 17, 2017 at 3:04 PM, Simon Spero <sesunc...@gmail.com> wrote:

On Aug 15, 2017 8:01 PM, "Gilles" <gil...@harfang.homelinux.org> wrote:

Saying that making record mutable is "breaking" is a bit unfair when we
do
> NOT document the mutability of the class in the first place.
>

I'm stating a fact: class is currently immutable, change would make it
mutable; it is functionally breaking.
I didn't say that you are forbidden to do it; just that it would be
unwise,
particularly if it would be to save a few bytes.


Exactly.

TL;DR. This is almost always a breaking semantic change; the safest ways of implementing it are binary breaking; it's unlikely to have a major performance impact; it might be better to create a new API module for
enhancements, with current package as legacy or implementation.

If a class previously exposed no mutators, adding one is usually a major change. This is especially true for final classes, but it still affects
use
cases where an instance is owned by another class, which may rely on the
lack of mutability to avoid making defensive copies.
Of course, a final class that has a package-private getter to a shared
copy of its backing array could be considered to be sending mixed
messages...

It is possible that a mutable class might have significant performance advantages over an immutable one beyond saving a few bytes. For example,
if
the updates are simple, and depend on the previous value of the cell,
then
a mutable version might have better cache behavior. If there's other sources of cache pressure this might have a higher than expected impact.
The costs of copying the original values might also be relatively
significant.

For an ETL use case these issues are unlikely to be limiting factors;
for a
start, there's a non-zero chance that a CSVRecord was extracted by parsing a CSV file. Also a transform will require conversion to some sort
of Number (or String allocation).

The current API doesn't easily support adding alternate implementations
of
the relevant types. Implementation classes are final, and important
return
types are concrete.

One solution might be to treat the current code as almost an
implementation
module, define a separate API module, and add extra interfaces and alternate implementations to support the target use case (mutable records, streams, reactivex, transform functions or what have you).

Simon





---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org

Reply via email to