Skip to content

Conversation

@alexmoore
Copy link
Contributor

@alexmoore alexmoore commented Dec 7, 2016

Adds the Blob data type and support for the ever-expanding DESCRIBE table result.

Description

Adds support for the new BLOB TS data type, and accepts another column for the DESCRIBE table result.

Related Issue

https://bashoeng.atlassian.net/browse/CLIENTS-935

Motivation and Context

To implement all the things!

How Has This Been Tested?

Tests run fine against TS 1.5 RC2.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Javadoc comments for any new public classes, interfaces, etc.
  • A basho_docs PR for new or changed features (basho/basho_docs#???)

Pull requests that are small and limited in scope are most welcome.

String s = new String(v.binaryValue(), StandardCharsets.UTF_8);
return new Cell(s);
final OtpErlangBinary v = (OtpErlangBinary) cell;
if (columnDescriptions.get(j).getType() == RiakTsPB.TsColumnType.VARCHAR)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO better to throw exception in case when something is incompatible, rather then silently skip it:

switch(columnDescriptions.get(j).getType()){
  case RiakTsPB.TsColumnType.VARCHAR://
  case RiakTsPB.TsColumnType.BLOB: //
  default:
    throw new IllegalStateException(<proper explanation>)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do fall through to the "default" case on line 367 / 368 where we throw an exception, but I guess it could be more specific.


if (lk.hasKeyOrder())
{
sb.append(" ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be (but not mandatory) re-written like:

sb.append(" ")
    .append(lk.getKeyOrder().toString());

@alexmoore alexmoore merged commit 5740e80 into develop Dec 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants