{"action":"created","comment":{"url":"https://api.github.com/repos/apache/arrow/pulls/comments/66692041","id":66692041,"diff_hunk":"@@ -34,85 +34,141 @@ namespace arrow {\n class Buffer;\n class MemoryPool;\n \n-struct CharType : public DataType {\n-  int size;\n-\n-  explicit CharType(int size) : DataType(Type::CHAR), size(size) {}\n-\n-  CharType(const CharType& other) : CharType(other.size) {}\n-\n-  virtual std::string ToString() const;\n-};\n-\n-// Variable-length, null-terminated strings, up to a certain length\n-struct VarcharType : public DataType {\n-  int size;\n-\n-  explicit VarcharType(int size) : DataType(Type::VARCHAR), size(size) {}\n-  VarcharType(const VarcharType& other) : VarcharType(other.size) {}\n-\n-  virtual std::string ToString() const;\n-};\n-\n-// TODO(wesm): add a BinaryArray layer in between\n-class StringArray : public ListArray {\n+class BinaryArray : public ListArray {\n  public:\n-  StringArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& offsets,\n+  BinaryArray(int32_t length, const std::shared_ptr<Buffer>& offsets,\n       const ArrayPtr& values, int32_t null_count = 0,\n-      const std::shared_ptr<Buffer>& null_bitmap = nullptr)\n-      : ListArray(type, length, offsets, values, null_count, null_bitmap) {\n-    // For convenience\n-    bytes_ = static_cast<UInt8Array*>(values.get());\n-    raw_bytes_ = bytes_->raw_data();\n-  }\n-\n-  StringArray(int32_t length, const std::shared_ptr<Buffer>& offsets,\n+      const std::shared_ptr<Buffer>& null_bitmap = nullptr);\n+  // Constructor that allows sub-classes/builders to propagate there logical type up the\n+  // class hierarchy.\n+  BinaryArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& offsets,\n       const ArrayPtr& values, int32_t null_count = 0,\n       const std::shared_ptr<Buffer>& null_bitmap = nullptr);\n \n-  // Compute the pointer t\n+  // Return the pointer to the given elements bytes\n+  // TODO(emkornfield) introduce a StringPiece or something similar to capture zero-copy\n+  // pointer + offset\n   const uint8_t* GetValue(int i, int32_t* out_length) const {\n-    int32_t pos = offsets_[i];\n+    DCHECK(out_length);\n+    const int32_t pos = offsets_[i];\n     *out_length = offsets_[i + 1] - pos;\n     return raw_bytes_ + pos;\n   }\n \n+  Status Validate() const override;\n+\n+ private:\n+  UInt8Array* bytes_;\n+  const uint8_t* raw_bytes_;\n+};\n+\n+class StringArray : public BinaryArray {\n+ public:\n+  StringArray(int32_t length, const std::shared_ptr<Buffer>& offsets,\n+      const ArrayPtr& values, int32_t null_count = 0,\n+      const std::shared_ptr<Buffer>& null_bitmap = nullptr);\n+  // Constructor that allows overriding the logical type, so subclasses can propagate\n+  // there\n+  // up the class hierarchy.\n+  StringArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& offsets,\n+      const ArrayPtr& values, int32_t null_count = 0,\n+      const std::shared_ptr<Buffer>& null_bitmap = nullptr)\n+      : BinaryArray(type, length, offsets, values, null_count, null_bitmap) {}\n+\n   // Construct a std::string\n+  // TODO: std::bad_alloc possibility\n   std::string GetString(int i) const {\n     int32_t nchars;\n     const uint8_t* str = GetValue(i, &nchars);\n     return std::string(reinterpret_cast<const char*>(str), nchars);\n   }\n \n+  Status Validate() const override;\n+};\n+\n+class CharArray : public StringArray {\n+ public:\n+  CharArray(const TypePtr& type, int32_t length, const std::shared_ptr<Buffer>& offsets,\n+      const ArrayPtr& values, int32_t null_count = 0,\n+      const std::shared_ptr<Buffer>& null_bitmap = nullptr)\n+      : StringArray(type, length, offsets, values, null_count, null_bitmap),\n+        char_type(std::dynamic_pointer_cast<CharType>(type).get()) {}\n+\n+  // The fixed size of each string\n+  // (TODO), right now this is in bytes, DB systems seems to\n+  // generally store this as characters.  Which would mean 3-6x the number of bytes\n+  // stored)\n+  int32_t string_size() const { return char_type->size; }\n+  Status Validate() const override;\n+\n  private:\n-  UInt8Array* bytes_;\n-  const uint8_t* raw_bytes_;\n+  // for convenience\n+  CharType* char_type;\n };\n \n-// String builder\n-class StringBuilder : public ListBuilder {\n+// BinaryBuilder : public ListBuilder\n+class BinaryBuilder : public ListBuilder {\n  public:\n-  explicit StringBuilder(MemoryPool* pool, const TypePtr& type)\n+  explicit BinaryBuilder(MemoryPool* pool, const TypePtr& type)\n       : ListBuilder(pool, std::make_shared<UInt8Builder>(pool, value_type_), type) {\n     byte_builder_ = static_cast<UInt8Builder*>(value_builder_.get());\n   }\n \n+  Status Append(const uint8_t* value, int32_t length) {\n+    RETURN_NOT_OK(ListBuilder::Append());\n+    return byte_builder_->Append(value, length);\n+  }\n+\n+  std::shared_ptr<Array> Finish() override {\n+    return ListBuilder::Transfer<BinaryArray>();\n+  }\n+\n+ protected:\n+  UInt8Builder* byte_builder_;\n+  static TypePtr value_type_;\n+};\n+\n+// String builder\n+class StringBuilder : public BinaryBuilder {\n+ public:\n+  explicit StringBuilder(MemoryPool* pool, const TypePtr& type)\n+      : BinaryBuilder(pool, type) {}\n+\n   Status Append(const std::string& value) { return Append(value.c_str(), value.size()); }\n \n   Status Append(const char* value, int32_t length) {\n-    RETURN_NOT_OK(ListBuilder::Append());\n-    return byte_builder_->Append(reinterpret_cast<const uint8_t*>(value), length);\n+    return BinaryBuilder::Append(reinterpret_cast<const uint8_t*>(value), length);\n   }\n+\n   Status Append(const std::vector<std::string>& values, uint8_t* null_bytes);\n \n   std::shared_ptr<Array> Finish() override {\n     return ListBuilder::Transfer<StringArray>();\n   }\n+};\n \n- protected:\n-  UInt8Builder* byte_builder_;\n+// Fixed width char builder\n+class CharBuilder : public StringBuilder {\n+ public:\n+  explicit CharBuilder(MemoryPool* pool, const std::shared_ptr<CharType>& type)\n+      : StringBuilder(pool, std::static_pointer_cast<DataType>(type)) {\n+    DCHECK(type);\n+    fixed_length_ = type->size;\n+  }\n \n-  static TypePtr value_type_;\n+  Status Append(const std::string& value) { return Append(value.c_str(), value.size()); }\n+\n+  Status Append(const char* value, int32_t length) {\n+    if (length != fixed_length_) {\n+      return Status::Invalid(\"Invalid length specified for fixed character type\");\n+    }\n+    return BinaryBuilder::Append(reinterpret_cast<const uint8_t*>(value), length);","path":"cpp/src/arrow/types/string.h","position":175,"original_position":175,"commit_id":"5af76ea34efe5587a1485cffefaf81fb4ed79f7f","original_commit_id":"0fe2e088801d28e54957b95938d97725ef701e36","user":{"login":"wesm","id":329591,"avatar_url":"https://avatars.githubusercontent.com/u/329591?v=3","gravatar_id":"","url":"https://api.github.com/users/wesm","html_url":"https://github.com/wesm","followers_url":"https://api.github.com/users/wesm/followers","following_url":"https://api.github.com/users/wesm/following{/other_user}","gists_url":"https://api.github.com/users/wesm/gists{/gist_id}","starred_url":"https://api.github.com/users/wesm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wesm/subscriptions","organizations_url":"https://api.github.com/users/wesm/orgs","repos_url":"https://api.github.com/users/wesm/repos","events_url":"https://api.github.com/users/wesm/events{/privacy}","received_events_url":"https://api.github.com/users/wesm/received_events","type":"User","site_admin":false},"body":"For simplicity's sake (I'm not sure where generic fixed-length lists would occur in practice? Although it's conceivable), we could define `class FixedLengthBinaryType : public PrimitiveType` and associated array containers as a way to accommodate other systems (e.g. Parquet, or things that work with wide decimal types). ","created_at":"2016-06-10T22:57:53Z","updated_at":"2016-06-10T22:57:53Z","html_url":"https://github.com/apache/arrow/pull/85#discussion_r66692041","pull_request_url":"https://api.github.com/repos/apache/arrow/pulls/85","_links":{"self":{"href":"https://api.github.com/repos/apache/arrow/pulls/comments/66692041"},"html":{"href":"https://github.com/apache/arrow/pull/85#discussion_r66692041"},"pull_request":{"href":"https://api.github.com/repos/apache/arrow/pulls/85"}}},"pull_request":{"url":"https://api.github.com/repos/apache/arrow/pulls/85","id":72474149,"html_url":"https://github.com/apache/arrow/pull/85","diff_url":"https://github.com/apache/arrow/pull/85.diff","patch_url":"https://github.com/apache/arrow/pull/85.patch","issue_url":"https://api.github.com/repos/apache/arrow/issues/85","number":85,"state":"open","locked":false,"title":"ARROW-210:  Cleanup of the string related types in C++ code base","user":{"login":"emkornfield","id":17869838,"avatar_url":"https://avatars.githubusercontent.com/u/17869838?v=3","gravatar_id":"","url":"https://api.github.com/users/emkornfield","html_url":"https://github.com/emkornfield","followers_url":"https://api.github.com/users/emkornfield/followers","following_url":"https://api.github.com/users/emkornfield/following{/other_user}","gists_url":"https://api.github.com/users/emkornfield/gists{/gist_id}","starred_url":"https://api.github.com/users/emkornfield/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emkornfield/subscriptions","organizations_url":"https://api.github.com/users/emkornfield/orgs","repos_url":"https://api.github.com/users/emkornfield/repos","events_url":"https://api.github.com/users/emkornfield/events{/privacy}","received_events_url":"https://api.github.com/users/emkornfield/received_events","type":"User","site_admin":false},"body":"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).\r\n\r\nI 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.","created_at":"2016-06-03T08:55:02Z","updated_at":"2016-06-10T22:57:53Z","closed_at":null,"merged_at":null,"merge_commit_sha":"fdda8bd0694309a79c305b6ca234428d0a3c8c8c","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/apache/arrow/pulls/85/commits","review_comments_url":"https://api.github.com/repos/apache/arrow/pulls/85/comments","review_comment_url":"https://api.github.com/repos/apache/arrow/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apache/arrow/issues/85/comments","statuses_url":"https://api.github.com/repos/apache/arrow/statuses/5af76ea34efe5587a1485cffefaf81fb4ed79f7f","head":{"label":"emkornfield:emk_string_types_wip","ref":"emk_string_types_wip","sha":"5af76ea34efe5587a1485cffefaf81fb4ed79f7f","user":{"login":"emkornfield","id":17869838,"avatar_url":"https://avatars.githubusercontent.com/u/17869838?v=3","gravatar_id":"","url":"https://api.github.com/users/emkornfield","html_url":"https://github.com/emkornfield","followers_url":"https://api.github.com/users/emkornfield/followers","following_url":"https://api.github.com/users/emkornfield/following{/other_user}","gists_url":"https://api.github.com/users/emkornfield/gists{/gist_id}","starred_url":"https://api.github.com/users/emkornfield/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emkornfield/subscriptions","organizations_url":"https://api.github.com/users/emkornfield/orgs","repos_url":"https://api.github.com/users/emkornfield/repos","events_url":"https://api.github.com/users/emkornfield/events{/privacy}","received_events_url":"https://api.github.com/users/emkornfield/received_events","type":"User","site_admin":false},"repo":{"id":54009687,"name":"arrow","full_name":"emkornfield/arrow","owner":{"login":"emkornfield","id":17869838,"avatar_url":"https://avatars.githubusercontent.com/u/17869838?v=3","gravatar_id":"","url":"https://api.github.com/users/emkornfield","html_url":"https://github.com/emkornfield","followers_url":"https://api.github.com/users/emkornfield/followers","following_url":"https://api.github.com/users/emkornfield/following{/other_user}","gists_url":"https://api.github.com/users/emkornfield/gists{/gist_id}","starred_url":"https://api.github.com/users/emkornfield/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/emkornfield/subscriptions","organizations_url":"https://api.github.com/users/emkornfield/orgs","repos_url":"https://api.github.com/users/emkornfield/repos","events_url":"https://api.github.com/users/emkornfield/events{/privacy}","received_events_url":"https://api.github.com/users/emkornfield/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/emkornfield/arrow","description":"Mirror of Apache Arrow","fork":true,"url":"https://api.github.com/repos/emkornfield/arrow","forks_url":"https://api.github.com/repos/emkornfield/arrow/forks","keys_url":"https://api.github.com/repos/emkornfield/arrow/keys{/key_id}","collaborators_url":"https://api.github.com/repos/emkornfield/arrow/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/emkornfield/arrow/teams","hooks_url":"https://api.github.com/repos/emkornfield/arrow/hooks","issue_events_url":"https://api.github.com/repos/emkornfield/arrow/issues/events{/number}","events_url":"https://api.github.com/repos/emkornfield/arrow/events","assignees_url":"https://api.github.com/repos/emkornfield/arrow/assignees{/user}","branches_url":"https://api.github.com/repos/emkornfield/arrow/branches{/branch}","tags_url":"https://api.github.com/repos/emkornfield/arrow/tags","blobs_url":"https://api.github.com/repos/emkornfield/arrow/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/emkornfield/arrow/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/emkornfield/arrow/git/refs{/sha}","trees_url":"https://api.github.com/repos/emkornfield/arrow/git/trees{/sha}","statuses_url":"https://api.github.com/repos/emkornfield/arrow/statuses/{sha}","languages_url":"https://api.github.com/repos/emkornfield/arrow/languages","stargazers_url":"https://api.github.com/repos/emkornfield/arrow/stargazers","contributors_url":"https://api.github.com/repos/emkornfield/arrow/contributors","subscribers_url":"https://api.github.com/repos/emkornfield/arrow/subscribers","subscription_url":"https://api.github.com/repos/emkornfield/arrow/subscription","commits_url":"https://api.github.com/repos/emkornfield/arrow/commits{/sha}","git_commits_url":"https://api.github.com/repos/emkornfield/arrow/git/commits{/sha}","comments_url":"https://api.github.com/repos/emkornfield/arrow/comments{/number}","issue_comment_url":"https://api.github.com/repos/emkornfield/arrow/issues/comments{/number}","contents_url":"https://api.github.com/repos/emkornfield/arrow/contents/{+path}","compare_url":"https://api.github.com/repos/emkornfield/arrow/compare/{base}...{head}","merges_url":"https://api.github.com/repos/emkornfield/arrow/merges","archive_url":"https://api.github.com/repos/emkornfield/arrow/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/emkornfield/arrow/downloads","issues_url":"https://api.github.com/repos/emkornfield/arrow/issues{/number}","pulls_url":"https://api.github.com/repos/emkornfield/arrow/pulls{/number}","milestones_url":"https://api.github.com/repos/emkornfield/arrow/milestones{/number}","notifications_url":"https://api.github.com/repos/emkornfield/arrow/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/emkornfield/arrow/labels{/name}","releases_url":"https://api.github.com/repos/emkornfield/arrow/releases{/id}","deployments_url":"https://api.github.com/repos/emkornfield/arrow/deployments","created_at":"2016-03-16T07:22:02Z","updated_at":"2016-03-16T07:22:05Z","pushed_at":"2016-06-07T08:37:31Z","git_url":"git://github.com/emkornfield/arrow.git","ssh_url":"git@github.com:emkornfield/arrow.git","clone_url":"https://github.com/emkornfield/arrow.git","svn_url":"https://github.com/emkornfield/arrow","homepage":null,"size":1100,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"apache:master","ref":"master","sha":"65740950c852b82c475ca84e970e147d25d27398","user":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"repo":{"id":51905353,"name":"arrow","full_name":"apache/arrow","owner":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/apache/arrow","description":"Mirror of Apache Arrow","fork":false,"url":"https://api.github.com/repos/apache/arrow","forks_url":"https://api.github.com/repos/apache/arrow/forks","keys_url":"https://api.github.com/repos/apache/arrow/keys{/key_id}","collaborators_url":"https://api.github.com/repos/apache/arrow/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/apache/arrow/teams","hooks_url":"https://api.github.com/repos/apache/arrow/hooks","issue_events_url":"https://api.github.com/repos/apache/arrow/issues/events{/number}","events_url":"https://api.github.com/repos/apache/arrow/events","assignees_url":"https://api.github.com/repos/apache/arrow/assignees{/user}","branches_url":"https://api.github.com/repos/apache/arrow/branches{/branch}","tags_url":"https://api.github.com/repos/apache/arrow/tags","blobs_url":"https://api.github.com/repos/apache/arrow/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/apache/arrow/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/apache/arrow/git/refs{/sha}","trees_url":"https://api.github.com/repos/apache/arrow/git/trees{/sha}","statuses_url":"https://api.github.com/repos/apache/arrow/statuses/{sha}","languages_url":"https://api.github.com/repos/apache/arrow/languages","stargazers_url":"https://api.github.com/repos/apache/arrow/stargazers","contributors_url":"https://api.github.com/repos/apache/arrow/contributors","subscribers_url":"https://api.github.com/repos/apache/arrow/subscribers","subscription_url":"https://api.github.com/repos/apache/arrow/subscription","commits_url":"https://api.github.com/repos/apache/arrow/commits{/sha}","git_commits_url":"https://api.github.com/repos/apache/arrow/git/commits{/sha}","comments_url":"https://api.github.com/repos/apache/arrow/comments{/number}","issue_comment_url":"https://api.github.com/repos/apache/arrow/issues/comments{/number}","contents_url":"https://api.github.com/repos/apache/arrow/contents/{+path}","compare_url":"https://api.github.com/repos/apache/arrow/compare/{base}...{head}","merges_url":"https://api.github.com/repos/apache/arrow/merges","archive_url":"https://api.github.com/repos/apache/arrow/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/apache/arrow/downloads","issues_url":"https://api.github.com/repos/apache/arrow/issues{/number}","pulls_url":"https://api.github.com/repos/apache/arrow/pulls{/number}","milestones_url":"https://api.github.com/repos/apache/arrow/milestones{/number}","notifications_url":"https://api.github.com/repos/apache/arrow/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/apache/arrow/labels{/name}","releases_url":"https://api.github.com/repos/apache/arrow/releases{/id}","deployments_url":"https://api.github.com/repos/apache/arrow/deployments","created_at":"2016-02-17T08:00:23Z","updated_at":"2016-06-09T23:40:47Z","pushed_at":"2016-06-10T22:08:42Z","git_url":"git://github.com/apache/arrow.git","ssh_url":"git@github.com:apache/arrow.git","clone_url":"https://github.com/apache/arrow.git","svn_url":"https://github.com/apache/arrow","homepage":null,"size":1386,"stargazers_count":570,"watchers_count":570,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":120,"mirror_url":"git://git.apache.org/arrow.git","open_issues_count":7,"forks":120,"open_issues":7,"watchers":570,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/apache/arrow/pulls/85"},"html":{"href":"https://github.com/apache/arrow/pull/85"},"issue":{"href":"https://api.github.com/repos/apache/arrow/issues/85"},"comments":{"href":"https://api.github.com/repos/apache/arrow/issues/85/comments"},"review_comments":{"href":"https://api.github.com/repos/apache/arrow/pulls/85/comments"},"review_comment":{"href":"https://api.github.com/repos/apache/arrow/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apache/arrow/pulls/85/commits"},"statuses":{"href":"https://api.github.com/repos/apache/arrow/statuses/5af76ea34efe5587a1485cffefaf81fb4ed79f7f"}}}}
{"actor":{"display_login":"wesm"}}