[
https://issues.apache.org/jira/browse/PHOENIX-1409?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14253075#comment-14253075
]
James Taylor commented on PHOENIX-1409:
---------------------------------------
Thanks for the revisions, [~ayingshu]. Here's some feedback:
- Rather than remove this part of the test, it should be turned into a positive
test (as it's valid now):
{code}
HTableDescriptor htableDesciptor1 =
htable1.getTableDescriptor();
HColumnDescriptor hcolumnDescriptor1 =
htableDesciptor1.getFamily(QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES);
assertNotNull(hcolumnDescriptor1);
-
- try {
-
- conn.createStatement().execute("ALTER TABLE TEST_TABLE SET
IN_MEMORY=false");
- fail("Should have caught exception.");
-
- } catch (SQLException e) {
- assertTrue(e.getMessage(), e.getMessage().contains("ERROR
1025 (42Y84): Unsupported property set in ALTER TABLE command."));
- }
- }finally {
{code}
- Looks like your indenting is off here, as there should be no diff (in
AlterTableIT):
{code}
- Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
- String ddl = "CREATE TABLE T (\n"
- +"ID1 VARCHAR(15) NOT NULL,\n"
- +"ID2 VARCHAR(15) NOT NULL,\n"
- +"CREATED_DATE DATE,\n"
- +"CREATION_TIME BIGINT,\n"
- +"LAST_USED DATE,\n"
- +"CONSTRAINT PK PRIMARY KEY (ID1, ID2)) SALT_BUCKETS = 8";
- Connection conn1 = DriverManager.getConnection(getUrl(), props);
- conn1.createStatement().execute(ddl);
- ddl = "ALTER TABLE T ADD CF.STRING VARCHAR";
- conn1.createStatement().execute(ddl);
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String ddl = "CREATE TABLE T (\n"
+ +"ID1 VARCHAR(15) NOT NULL,\n"
+ +"ID2 VARCHAR(15) NOT NULL,\n"
+ +"CREATED_DATE DATE,\n"
+ +"CREATION_TIME BIGINT,\n"
+ +"LAST_USED DATE,\n"
+ +"CONSTRAINT PK PRIMARY KEY (ID1, ID2)) SALT_BUCKETS =
8";
+ Connection conn1 = DriverManager.getConnection(getUrl(), props);
+ conn1.createStatement().execute(ddl);
+ ddl = "ALTER TABLE T ADD CF.STRING VARCHAR";
+ conn1.createStatement().execute(ddl);
{code}
- Why is this new check here? It's ok to add a new column family in an add
column call.
{code}
+ if (columnFamilyProps != null && !columnFamilyProps.isEmpty()){
+ if (!checkColumnFamilyExistence(Bytes.toBytes(tableName),
columnFamilyProps)) {
+ throw new
SQLExceptionInfo.Builder(SQLExceptionCode.COLUMN_FAMILY_NOT_FOUND).build().buildException();
+ }
+ }
{code}
- Can you add more testing in AlterTableIT to cover setting column family
properties, setting table properties, setting Phoenix table properties across
single and multiple column family scenarios?
- Here in MetaDataClient, make defaultColDescriptor a static final and just use
internally in isHTableProperty. No need to instantiate it again and again.
{code}
+ HColumnDescriptor defaultColDescriptor = new
HColumnDescriptor(QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES);
+ for (String family : stmtPropsMap.keySet()) {
+ // if there is no column family specified, then use the default
column family name.
+ byte[] famBytes = Strings.isNullOrEmpty(family) ?
QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES : Bytes.toBytes(family);
+ List<Pair<String, Object>> propsList = stmtPropsMap.get(family);
+ Map<String, Object> colFamilyPropsMap = new HashMap<String,
Object>(propsList.size());
+ Map<String, Object> ttlPropsMap = new HashMap<String,
Object>(propsList.size());
+ for (Pair<String, Object> prop : propsList) {
+ String propName = prop.getFirst();
+ validateIfAllowedInAlterTable(propName);
+ if (family.isEmpty() &&
propName.equalsIgnoreCase("TTL")) {
+ settingTTL = true;
+ ttlPropsMap.put(propName, prop.getSecond());
+ } else if (isHTableProperty(propName,
defaultColDescriptor)) {
{code}
- In general, create static constants for all String property names, for
example here for "TTL" (or better yet use existing HBase constants):
{code}
+ if (family.isEmpty() &&
propName.equalsIgnoreCase("TTL")) {
{code}
- I'm confused by this code, as IS_IMMUTABLE_ROWS_PROP_NAME, MULTI_TENANT, etc
are not HTable property names, yet a check for them is in this else if
statement:
{code}
+ } else if (isHTableProperty(propName,
defaultColDescriptor)) {
+ if
(!family.equals(QueryConstants.ALL_FAMILY_PROPERTIES_KEY)) {
+ throw new
SQLExceptionInfo.Builder(SQLExceptionCode.COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY)
+ .setMessage("Column Family: " + family
+ ", Property: " + propName).build()
+ .buildException();
+ }
+ if
(propName.equals(PTable.IS_IMMUTABLE_ROWS_PROP_NAME)) {
+ isImmutableRowsProp =
(Boolean)prop.getSecond();
+ } else if
(propName.equals(PhoenixDatabaseMetaData.MULTI_TENANT)) {
+ multiTenantProp =
(Boolean)prop.getSecond();
+ } else if (propName.equals(DISABLE_WAL)) {
+ disableWALProp =
(Boolean)prop.getSecond();
+ }
+ tableProps.put(propName, prop.getSecond());
+ } else {
{code}
- This seems to have diverged from Samarth's WIP patch quite a bit. Overall,
this seems to be making the code more complex, but I think we want the opposite
effect.
> Allow ALTER TABLE <table> SET command to update HTableDescriptor and
> HColumnDescriptor properties
> -------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-1409
> URL: https://issues.apache.org/jira/browse/PHOENIX-1409
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 4.2
> Reporter: James Taylor
> Assignee: Alicia Ying Shu
> Attachments: Phoenix-1409-v1.patch, Phoenix-1409.patch, WIP.patch
>
>
> Once PHOENIX-1408 is fixed, we should allow HTableDescriptor and
> HColumnDescriptor properties through the ALTER TABLE <table> SET command.
> It'd just be a matter of passing these properties through the existing
> methods, as we support this for CREATE TABLE.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
