Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tiamnu): hard code in defs.h (#1481) #1533

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

zzzz-vincent
Copy link
Contributor

change magic number to readable const

Summary about this PR

Issue Number: close #1481

  1. change some magic number to readable const variable
  2. move some const value related to column and DPN from common/def.h to core/column_share.h

Tests Check List

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Changelog

  • New Feature
  • Bug Fix
  • Performance Improvement
  • Build/Testing/CI/CD
  • Documentation
  • Not for changelog (changelog entry is not required)

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features

@mergify
Copy link
Contributor

mergify bot commented Apr 6, 2023

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the PR-feature feature for pull request label Apr 6, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.20 ⚠️

Comparison is base (8e197ae) 45.15% compared to head (41e4250) 44.96%.

❗ Current head 41e4250 differs from pull request most recent head 2e74d03. Consider uploading reports for the commit 2e74d03 to get more accurate results

Additional details and impacted files
@@                 Coverage Diff                 @@
##           stonedb-5.7-dev    #1533      +/-   ##
===================================================
- Coverage            45.15%   44.96%   -0.20%     
===================================================
  Files                 1999     1999              
  Lines               413931   413915      -16     
===================================================
- Hits                186906   186108     -798     
- Misses              227025   227807     +782     
Impacted Files Coverage Δ
storage/tianmu/core/table_share.h 77.77% <ø> (ø)
storage/tianmu/core/column_share.cpp 85.60% <75.00%> (ø)
storage/tianmu/core/column_share.h 100.00% <100.00%> (ø)
storage/tianmu/core/tianmu_attr.cpp 69.55% <100.00%> (ø)

... and 293 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@RingsC RingsC left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@hustjieke hustjieke left a comment

Choose a reason for hiding this comment

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

LGTM, nice job!

@hustjieke
Copy link
Collaborator

ci/cd failed, in debian:

[ 19%] tianmu.alter_table_mix_use               w10 [ fail ]
        Test ended at 2023-04-09 03:59:47

CURRENT_TEST: tianmu.alter_table_mix_use
mysqltest: At line 82: query 'alter table alter_table_mix_test rename alter_table_rename_drop, drop c_longblob' failed: 1030: Got error 1 from storage engine

The result from queries just before the failure was:
< snip >
  `c_tinyint` tinyint(4) DEFAULT NULL COMMENT 'tinyint',
  `c_smallint` smallint(6) NOT NULL COMMENT 'smallint',
  `c_mediumint` mediumint(9) DEFAULT NULL COMMENT 'mediumint',
  `c_int` int(11) DEFAULT NULL COMMENT 'int',
  `c_bigint` bigint(20) DEFAULT NULL COMMENT 'bigint',
  `c_float` float DEFAULT NULL COMMENT 'float',
  `c_double` double DEFAULT NULL COMMENT 'double',
  `c_decimal` decimal(10,5) DEFAULT NULL COMMENT 'decimal',
  `c_date` date DEFAULT NULL COMMENT 'date',
  `c_datetime` datetime DEFAULT NULL COMMENT 'datetime',
  `c_timestamp` timestamp NULL DEFAULT NULL COMMENT 'timestamp',
  `c_time` time DEFAULT NULL COMMENT 'time',
  `c_char` char(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'char',
  `c_varchar` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'varchar',
  `c_blob` blob COMMENT 'blob',
  `c_text` text COLLATE utf8mb4_unicode_ci COMMENT 'text',
  `c_longblob` longblob COMMENT 'longblob'
) ENGINE=TIANMU DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci
insert into alter_table_mix_test(c_tinyint,c_smallint,c_mediumint,c_int,c_bigint,c_float,c_double,c_decimal,c_date,c_datetime,c_timestamp,c_time,c_char,c_varchar,c_text)
values(105,105,105,105,105,5.2,10.88,105.083,'2016-02-25','2016-02-25 10:20:01','2016-02-25 05:20:01','10:20:01','stoneatom1','hello1','bcdefghijklmn');
safe_process[46047]: Child process: 46048, exit: 1

 - the logfile can be found in '/stonedb57/install/mysql-test/var/log/tianmu.alter_table_mix_use/alter_table_mix_use.log'
[ 19%] tianmu.issue468                          w7 [ pass ]    216

In Centos7:

[ 61%] tianmu.alter_table1                      w1 [ fail ]
        Test ended at 2023-04-09 03:59:03

CURRENT_TEST: tianmu.alter_table1
mysqltest: At line 64: query 'alter table st2 order by deptId,id' failed: 6: An unknown system exception error caught.

The result from queries just before the failure was:
< snip >
salary FLOAT
);
insert into st2 values(3,'haha1',45,4.5);
insert into st2 values(1,'haha2',12,1.2);
insert into st2 values(4,'haha3',31,3.2);
insert into st2 values(2,'haha4',55,3.5);
alter table st2 order by id;
select * from st2;
id	name	deptId	salary
1	haha2	12	1.2
2	haha4	55	3.5
3	haha1	45	4.5
4	haha3	31	3.2
alter table st2 order by id,deptId;
select * from st2;
id	name	deptId	salary
1	haha2	12	1.2
2	haha4	55	3.5
3	haha1	45	4.5
4	haha3	31	3.2
safe_process[43892]: Child process: 43893, exit: 1

 - the logfile can be found in '/stonedb57/install/mysql-test/var/log/tianmu.alter_table1/alter_table1.log'
[ 61%] tianmu.std_test                          w5 [ pass ]   3744

It seems the changes caused some internal crashed. We should do further confirmation.
ping @zzzz-vincent.

@zzzz-vincent
Copy link
Contributor Author

zzzz-vincent commented Apr 10, 2023

This is very interesting. I looked into these two tests but couldn't really get where the error came from. But posted some preliminary investigation here incase you may know what happened.

In debian, the alter_table_mix_use failed when executing alter rename and drop. In centos, the test alter_table1 failed due to the third time alter reorder the table.

I tested locally, and two other tests failed: tianmu.alter_table_primarykey and tianmu.alter_table_primarykey which were all involved alter. But after I tested multiple times, some runs got all passed. Not sure why this change cause non-deterministic failures.

And after a very quick check, I think at lest some failures were thrown from type_conversion_status Item::save_in_field_inner(Field *field, bool no_conversions) but haven't yet track why.

Will look into this soon.

@mergify
Copy link
Contributor

mergify bot commented Apr 11, 2023

This pull request has merge conflicts, you should resolve it before merged. @zzzz-vincent please update it :)

Try @mergify update or update manually.

@Nliver Nliver added this to the stonedb_5.7_v1.0.4 milestone Apr 11, 2023
@RingsC
Copy link
Contributor

RingsC commented Apr 13, 2023

@zzzz-vincent , Pls, you need re-pull the latest code, and, we refined some files you changed, Thanks.

change magic number to readable const
@mergify mergify bot merged commit 45e75ee into stoneatom:stonedb-5.7-dev Apr 14, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-feature feature for pull request
Projects
Development

Successfully merging this pull request may close these issues.

feature: hard code in defs.h
4 participants