ARROW-210: Cleanup of the string related types in C++ code base

[emkornfield]

One thing that is worth discussing is if char types should also be removed (if they aren't i'll add the missing unit tests).

I also moved CharType to type.h which seems more consistent with existing code. I can clean it up either way in a follow-up review if we decide with want to push types into their corresponding Array headers.

[wesm]

I'm a little jammed up with Spark Summit this week, but I will review as soon as I can

[wesm]

UTF-8 in the context of CHAR is a bit of a quagmire, we can address in a separate discussion

[wesm]

I would be curious what level of consistency there is between RDBMS implementations or if we're going to have to pick a convention for arrow-cpp

[emkornfield]

http://docs.oracle.com/cd/B19306_01/server.102/b14225/ch7progrunicode.htm (maximum number of characters) x (maximum number of bytes for each character)

Postgres doesn't seem to distinguish types (https://www.postgresql.org/docs/9.0/static/datatype-character.html) i.e. it seems to always stores a length per entry

Mysql (http://dev.mysql.com/doc/refman/5.7/en/charset-unicode-utf8.html) To save space with UTF-8, use VARCHAR instead of CHAR. Otherwise, MySQL must reserve three bytes for each character in a CHAR CHARACTER SET utf8 column because that is the maximum possible character length. For example, MySQL must reserve 30 bytes for a CHAR(10) CHARACTER SET utf8 column.

MS SQL https://msdn.microsoft.com/en-us/library/ms186939.aspx "n defines the string length and must be a value from 1 through 4,000. The storage size is two times n bytes. When the collation code page uses double-byte characters, the storage size is still n bytes."

[emkornfield]

I agree, its a thorny topic. Would you be opposed to removing it for now along with varchar? It doesn't look like there is code on the java side that actually makes use of the type (BasicTypeHelper.java is the only reference to it aside fro Types.java and all of the FixedValue's appear to be commented out).

[wesm]

Let's remove CharType to avoid the unneeded complexity for now.

[wesm]

The code itself looks fine; there are a couple of rabbit holes we should investigate.

[wesm]

If this cast fails, the result is null. I'm not sure the best way to deal with this; DCHECK maybe?

[wesm]

OK, this all looks good (one minor dynamic_cast question). I think we can table the type hierarchy question for now as it has little user API impact. If we nix all the CHAR stuff then I can merge this

[emkornfield]

sounds good.

On Tue, Jun 14, 2016 at 8:01 PM, Wes McKinney notifications@github.com
wrote:

OK, this all looks good (one minor dynamic_cast question). I think we can
table the type hierarchy question for now as it has little user API impact.
If we nix all the CHAR stuff then I can merge this

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#85 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ARCsDmT3E8vukQ2ChsezFDtkruaeDv6oks5qL2smgaJpZM4ItVKY
.

[emkornfield]

The travis-CI failing seems to be environmental. I need to go AFK for the evening. I will try taking a closer look tomorrow

[wesm]

@emkornfield if you rebase you should get a green build now, then I can merge this

[wesm]

thanks! +1